cppgc: Optimize allocation by placing GCInfo in separate section [v8/v8 : main]
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
📍 Job mac-m1_mini_2020-perf/speedometer3 complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/14994d57490000
Related details
- Anton Bikineev
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
📍 Job mac-m1_mini_2020-perf/speedometer3 complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/11bb134f490000
Related details
- Anton Bikineev
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
📍 Job mac-m4-mini-perf/speedometer3 complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/127c3508c90000
Related details
- Anton Bikineev
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
📍 Job mac-m1_mini_2020-perf/speedometer3.crossbench complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/14938f7cc90000
Related details
- Anton Bikineev
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
📍 Job mac-m4-mini-perf/speedometer3.crossbench complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/10b0c36cc90000
Related details
- Anton Bikineev
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Anton Bikineev (Gerrit)
Anton Bikineev added 1 comment![]( "Open in Gerrit")
- Michael Lippautz
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Lippautz (Gerrit)
Michael Lippautz added 1 comment![]( "Open in Gerrit")
#if !defined(CPPGC_ENABLE_OBJECT_SECTION_GCINFO)High-level comment: Can we refactor this in a way that we can keep testing the table even in this configuration (to be able to easily switch) and change the top-level dispatch somehow that we use the segment?
Related details
- Anton Bikineev
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Anton Bikineev (Gerrit)
Anton Bikineev added 1 comment![]( "Open in Gerrit")
#if !defined(CPPGC_ENABLE_OBJECT_SECTION_GCINFO)High-level comment: Can we refactor this in a way that we can keep testing the table even in this configuration (to be able to easily switch) and change the top-level dispatch somehow that we use the segment?
I don't like conceptually having the table in the configuration with the section, I think it can be misleading. Having the table behind ifdef gaurds IMO makes it clear that the table is not used in this configuration. We still test the table on 32-bit and Windows.
change the top-level dispatch somehow that we use the segment?
I don't follow. We dispatch in GCInfoTrait (in API) and keep the GCInfoTable behind it. I don't think we want to move the table to API.
I kept the GCInfoTrait tests and added some section specific ones.
Related details
- Michael Lippautz
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Lippautz (Gerrit)
Michael Lippautz added 3 comments![]( "Open in Gerrit")
struct V8_EXPORT EnsureGCInfoIndexTrait final {This trait should also be ifdef'ed away?
Can we move this below in an #else branch?
class V8_EXPORT GlobalGCInfoTable final {I keep on wondering whethere the global table is a good abstraction for version where the info is in the elf section. I realize it's a global table but the split between trait and GlobalGCInfoTable seems a bit weird.
Mostly wondering because that would then allow to completely ifdef away all the table stuff for the section version.
Anton BikineevHigh-level comment: Can we refactor this in a way that we can keep testing the table even in this configuration (to be able to easily switch) and change the top-level dispatch somehow that we use the segment?
I don't like conceptually having the table in the configuration with the section, I think it can be misleading. Having the table behind ifdef gaurds IMO makes it clear that the table is not used in this configuration. We still test the table on 32-bit and Windows.
change the top-level dispatch somehow that we use the segment?
I don't follow. We dispatch in GCInfoTrait (in API) and keep the GCInfoTable behind it. I don't think we want to move the table to API.
I kept the GCInfoTrait tests and added some section specific ones.
Okay, fair enough. I was worried ab it about test coverage but at least for now we also have component builds to test this on e.g. Linux.
Related details
- Anton Bikineev
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Anton Bikineev (Gerrit)
Anton Bikineev added 2 comments![]( "Open in Gerrit")
This trait should also be ifdef'ed away?
Can we move this below in an #else branch?
Good point, done
class V8_EXPORT GlobalGCInfoTable final {I keep on wondering whethere the global table is a good abstraction for version where the info is in the elf section. I realize it's a global table but the split between trait and GlobalGCInfoTable seems a bit weird.
Mostly wondering because that would then allow to completely ifdef away all the table stuff for the section version.
We would still have ifdefs for these two approaches - notice that we don't even access the "table" with the section approach, we merely compute the diff between gcinfo and the start of the section.
Generally I agree that this abstraction could work for both approaches. You can think of the section approach and mmaping as two different strategies for the table allocation. I don't think, however, that this should be part of this CL, tbh.
Related details
- Michael Lippautz
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Lippautz (Gerrit)
Michael Lippautz added 1 comment![]( "Open in Gerrit")
class V8_EXPORT GlobalGCInfoTable final {Anton BikineevI keep on wondering whethere the global table is a good abstraction for version where the info is in the elf section. I realize it's a global table but the split between trait and GlobalGCInfoTable seems a bit weird.
Mostly wondering because that would then allow to completely ifdef away all the table stuff for the section version.
We would still have ifdefs for these two approaches - notice that we don't even access the "table" with the section approach, we merely compute the diff between gcinfo and the start of the section.
Generally I agree that this abstraction could work for both approaches. You can think of the section approach and mmaping as two different strategies for the table allocation. I don't think, however, that this should be part of this CL, tbh.
We would still have ifdefs for these two approaches - notice that we don't even access the "table" with the section approach, we merely compute the diff between gcinfo and the start of the section.
That's why I wondered if we should use the `GlobalGCInfoTable` at all with the sections.
Basically we use `__start_gc_info_section` in the trait and the table which I find confusing and hard to follow.
Conceptually `__start_gc_info_section` feels like a global table (nice!) which is why we should have it there. The trait uses it as well though which seems impossible to fix? Maybe if we forward to a `GCGlobalInfoTable::EnsureGCInfoIndex<T>` to instantiate the entry there?
I am not against this CL at all but I find it a bit hard to navigate with the 2 completely different approaches which is why I am trying to find clear entry points.
Related details
- Anton Bikineev
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Anton Bikineev (Gerrit)
Anton Bikineev added 1 comment![]( "Open in Gerrit")
class V8_EXPORT GlobalGCInfoTable final {Anton BikineevI keep on wondering whethere the global table is a good abstraction for version where the info is in the elf section. I realize it's a global table but the split between trait and GlobalGCInfoTable seems a bit weird.
Mostly wondering because that would then allow to completely ifdef away all the table stuff for the section version.
Michael LippautzWe would still have ifdefs for these two approaches - notice that we don't even access the "table" with the section approach, we merely compute the diff between gcinfo and the start of the section.
Generally I agree that this abstraction could work for both approaches. You can think of the section approach and mmaping as two different strategies for the table allocation. I don't think, however, that this should be part of this CL, tbh.
We would still have ifdefs for these two approaches - notice that we don't even access the "table" with the section approach, we merely compute the diff between gcinfo and the start of the section.
That's why I wondered if we should use the `GlobalGCInfoTable` at all with the sections.
Basically we use `__start_gc_info_section` in the trait and the table which I find confusing and hard to follow.
Conceptually `__start_gc_info_section` feels like a global table (nice!) which is why we should have it there. The trait uses it as well though which seems impossible to fix? Maybe if we forward to a `GCGlobalInfoTable::EnsureGCInfoIndex<T>` to instantiate the entry there?
I am not against this CL at all but I find it a bit hard to navigate with the 2 completely different approaches which is why I am trying to find clear entry points.
EnsureGCInfoIndex crosses the API, so the type is not available there - the instantantion should happen on the API side. For that we either move GlobalGCInfoTable there, or use some indirection that is called from both the trait and GlobalGCInfoTable. What do you think to wrap the functionality into a class, like GCInfoTableSection?
Related details
- Michael Lippautz
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Anton Bikineev (Gerrit)
Anton Bikineev added 1 comment![]( "Open in Gerrit")
class V8_EXPORT GlobalGCInfoTable final {Anton BikineevI keep on wondering whethere the global table is a good abstraction for version where the info is in the elf section. I realize it's a global table but the split between trait and GlobalGCInfoTable seems a bit weird.
Mostly wondering because that would then allow to completely ifdef away all the table stuff for the section version.
Michael LippautzWe would still have ifdefs for these two approaches - notice that we don't even access the "table" with the section approach, we merely compute the diff between gcinfo and the start of the section.
Generally I agree that this abstraction could work for both approaches. You can think of the section approach and mmaping as two different strategies for the table allocation. I don't think, however, that this should be part of this CL, tbh.
Anton BikineevWe would still have ifdefs for these two approaches - notice that we don't even access the "table" with the section approach, we merely compute the diff between gcinfo and the start of the section.
That's why I wondered if we should use the `GlobalGCInfoTable` at all with the sections.
Basically we use `__start_gc_info_section` in the trait and the table which I find confusing and hard to follow.
Conceptually `__start_gc_info_section` feels like a global table (nice!) which is why we should have it there. The trait uses it as well though which seems impossible to fix? Maybe if we forward to a `GCGlobalInfoTable::EnsureGCInfoIndex<T>` to instantiate the entry there?
I am not against this CL at all but I find it a bit hard to navigate with the 2 completely different approaches which is why I am trying to find clear entry points.
EnsureGCInfoIndex crosses the API, so the type is not available there - the instantantion should happen on the API side. For that we either move GlobalGCInfoTable there, or use some indirection that is called from both the trait and GlobalGCInfoTable. What do you think to wrap the functionality into a class, like GCInfoTableSection?
(wrapped the whole __start_gc_info_section operations in the GCInfoTableSection class)
Michael Lippautz (Gerrit)
Michael Lippautz voted and added 3 comments![]( "Open in Gerrit")
Votes added by Michael Lippautz
| Code-Review | +1 |
3 comments
lgtm, thanks for refactoring. I think this is now much better
inline HeapObjectName GetHiddenName(This should probably go outside as it's unrelated :)
class V8_EXPORT GlobalGCInfoTable final {Anton BikineevI keep on wondering whethere the global table is a good abstraction for version where the info is in the elf section. I realize it's a global table but the split between trait and GlobalGCInfoTable seems a bit weird.
Mostly wondering because that would then allow to completely ifdef away all the table stuff for the section version.
Michael LippautzWe would still have ifdefs for these two approaches - notice that we don't even access the "table" with the section approach, we merely compute the diff between gcinfo and the start of the section.
Generally I agree that this abstraction could work for both approaches. You can think of the section approach and mmaping as two different strategies for the table allocation. I don't think, however, that this should be part of this CL, tbh.
Anton BikineevWe would still have ifdefs for these two approaches - notice that we don't even access the "table" with the section approach, we merely compute the diff between gcinfo and the start of the section.
That's why I wondered if we should use the `GlobalGCInfoTable` at all with the sections.
Basically we use `__start_gc_info_section` in the trait and the table which I find confusing and hard to follow.
Conceptually `__start_gc_info_section` feels like a global table (nice!) which is why we should have it there. The trait uses it as well though which seems impossible to fix? Maybe if we forward to a `GCGlobalInfoTable::EnsureGCInfoIndex<T>` to instantiate the entry there?
I am not against this CL at all but I find it a bit hard to navigate with the 2 completely different approaches which is why I am trying to find clear entry points.
Anton BikineevEnsureGCInfoIndex crosses the API, so the type is not available there - the instantantion should happen on the API side. For that we either move GlobalGCInfoTable there, or use some indirection that is called from both the trait and GlobalGCInfoTable. What do you think to wrap the functionality into a class, like GCInfoTableSection?
(wrapped the whole __start_gc_info_section operations in the GCInfoTableSection class)
- Anton Bikineev
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Anton Bikineev (Gerrit)
Anton Bikineev added 2 comments![]( "Open in Gerrit")
This should probably go outside as it's unrelated :)
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Anton Bikineev (Gerrit)
Anton Bikineev voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
v8-scoped@luci-project-accounts.iam.gserviceaccount.com (Gerrit)
v8-s...@luci-project-accounts.iam.gserviceaccount.com submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
10 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: src/heap/cppgc/gc-info.cc
Insertions: 0, Deletions: 11.
@@ -11,17 +11,6 @@
namespace cppgc::internal {
-namespace {
-
-HeapObjectName GetHiddenName(
- const void*, HeapObjectNameForUnnamedObject name_retrieval_mode) {
- return {
- NameProvider::kHiddenName,
- name_retrieval_mode == HeapObjectNameForUnnamedObject::kUseHiddenName};
-}
-
-} // namespace
-
// static
GCInfoIndex EnsureGCInfoIndexTrait::EnsureGCInfoIndex(
std::atomic<GCInfoIndex>& registered_index, TraceCallback trace_callback,
```
```
The name of the file: include/cppgc/internal/gc-info.h
Insertions: 9, Deletions: 11.
@@ -15,8 +15,7 @@
#include "cppgc/trace-trait.h"
#include "v8config.h" // NOLINT(build/include_directory)
-namespace cppgc {
-namespace internal {
+namespace cppgc::internal {
using GCInfoIndex = uint16_t;
static constexpr GCInfoIndex kMaxGCInfoIndex = 1 << 14;
@@ -33,6 +32,13 @@
size_t padding = 0;
};
+inline HeapObjectName GetHiddenName(
+ const void*, HeapObjectNameForUnnamedObject name_retrieval_mode) {
+ return {
+ NameProvider::kHiddenName,
+ name_retrieval_mode == HeapObjectNameForUnnamedObject::kUseHiddenName};
+}
+
#if defined(CPPGC_ENABLE_OBJECT_SECTION_GCINFO)
#if defined(__APPLE__)
@@ -72,13 +78,6 @@
}
};
-inline HeapObjectName GetHiddenName(
- const void*, HeapObjectNameForUnnamedObject name_retrieval_mode) {
- return {
- NameProvider::kHiddenName,
- name_retrieval_mode == HeapObjectNameForUnnamedObject::kUseHiddenName};
-}
-
#else // defined(CPPGC_ENABLE_OBJECT_SECTION_GCINFO)
struct V8_EXPORT EnsureGCInfoIndexTrait final {
@@ -222,7 +221,6 @@
NameTrait<T>::HasNonHiddenName() ? NameTrait<T>::GetName : GetHiddenName);
#endif // defined(CPPGC_ENABLE_OBJECT_SECTION_GCINFO)
-} // namespace internal
-} // namespace cppgc
+} // namespace cppgc::internal
#endif // INCLUDE_CPPGC_INTERNAL_GC_INFO_H_
```
Change information
cppgc: Optimize allocation by placing GCInfo in separate section
Place GCInfo objects into a separate section (`gc_info_section` or
`__DATA,gc_info_section`) and compute the index directly as an offset
from the start of the section. This avoids the overhead of dynamic
registration and checking on allocation. This also eliminates the need
for `GCInfoTable` when active.
The optimization is enabled by default on desktop (Linux and macOS)
under the `cppgc_enable_object_section_gcinfo` GN flag for non-component
builds.
ELF and MachO platforms are supported.
TAG=agy
- M BUILD.bazel
- M BUILD.gn
- M gni/v8.gni
- M include/cppgc/internal/gc-info.h
- M src/heap/cppgc/gc-info-table.cc
- M src/heap/cppgc/gc-info-table.h
- M src/heap/cppgc/gc-info.cc
- M src/heap/cppgc/heap-object-header.h
- M test/unittests/heap/cppgc/gc-info-unittest.cc
- M test/unittests/heap/cppgc/heap-object-header-unittest.cc
Code-Review: +1 by Michael Lippautz
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |