Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1815153002:
Introduce EmbedderHeapTracer (Closed)
|
Created:
4 years, 9 months ago by Marcel Hlopko Modified:
4 years, 8 months ago CC:
Hannes Payer (out of office), Paweł Hajdan Jr., ulan, v8-reviews_googlegroups.com Base URL:
https://chromium.googlesource.com/v8/v8.git@master Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
DescriptionIntroduce EmbedderHeapTracer
BUG=468240
LOG=no
Committed: https://crrev.com/289f3824973714e8a917bdcd485fcd8cbb58fdfd
Cr-Commit-Position: refs/heads/master@{#35162}
Patch Set 1 #Patch Set 2 : Introduce EmbedderHeapTracer #Patch Set 3 : Fix formatting #Patch Set 4 : Fix formatting #
Total comments: 9
Patch Set 5 : Incorporate Jochen'c comments #
Total comments: 1
Patch Set 6 : Gave the api a week to settle down and this is the result #
Total comments: 20
Patch Set 7 : Incorporate Jochen's wonderful comments #
Total comments: 8
Patch Set 8 : Incorporated wonderful Hannes' comments #
Total comments: 2
Messages
Total messages: 26 (8 generated)
hlopko@chromium.org changed reviewers: + hpayer@chromium.org, jochen@chromium.org
ptal, this is needed for clearing dom tracing mark bits.
I think it would be cleaner if we had a dedicated interface the embedder needs to provide for marking instead of adding a bunch of "random" callbacks, wdyt?
Did you have something like this on your mind?
jochen@chromium.org changed reviewers: + ulan@chromium.org
yes, getting close :) https://codereview.chromium.org/1815153002/diff/60001/include/v8.h File include/v8.h (right): https://codereview.chromium.org/1815153002/diff/60001/include/v8.h#newcode5161 include/v8.h:5161: typedef void (*GCAliveHandleCallback)(Persistent<Object>* handle, why not add a new method on PersistentBase? https://codereview.chromium.org/1815153002/diff/60001/include/v8.h#newcode5364 include/v8.h:5364: class V8_EXPORT EmbedderHeapTracer { // NOLINT what is // NOLINT needed for? since the class doesn't have anything outside of this header, I don't think you need to V8_EXPORT it? https://codereview.chromium.org/1815153002/diff/60001/include/v8.h#newcode5366 include/v8.h:5366: virtual ~EmbedderHeapTracer() {} should this be protected? https://codereview.chromium.org/1815153002/diff/60001/include/v8.h#newcode5367 include/v8.h:5367: virtual void TraceWrappablesFrom(Isolate* isolate, Persistent<Object>* value, Do you expect this method to return immediately? Do you expect it gets invoked often? I wonder whether instead of passing a single persistent, we should pass a vector of internal fields (because that's essentially what the embedder needs to identify the object the handle points out). I'd also call it something like "PutOnMarkingDeque" or similar. in any case, please document what the method should do. also, should it be pure virtual? https://codereview.chromium.org/1815153002/diff/60001/include/v8.h#newcode5841 include/v8.h:5841: * This function removes the tracer installed by `SetEmbedderHeapTracer` please no backticks you could require the EmbedderHeapTracer* to be passed in again, so you can check that the embedder is removing the right one. however, I wonder whether we should allow removing it at all...
https://codereview.chromium.org/1815153002/diff/60001/include/v8.h File include/v8.h (right): https://codereview.chromium.org/1815153002/diff/60001/include/v8.h#newcode5161 include/v8.h:5161: typedef void (*GCAliveHandleCallback)(Persistent<Object>* handle, On 2016/03/21 13:31:15, jochen - slow wrote: > why not add a new method on PersistentBase? Done. https://codereview.chromium.org/1815153002/diff/60001/include/v8.h#newcode5364 include/v8.h:5364: class V8_EXPORT EmbedderHeapTracer { // NOLINT On 2016/03/21 13:31:15, jochen - slow wrote: > what is // NOLINT needed for? > > since the class doesn't have anything outside of this header, I don't think you > need to V8_EXPORT it? Done. https://codereview.chromium.org/1815153002/diff/60001/include/v8.h#newcode5367 include/v8.h:5367: virtual void TraceWrappablesFrom(Isolate* isolate, Persistent<Object>* value, I added the comment, made the method pure virtual. Regarding internal fields, sure can do, I just think passing persistent "feels" nicer and more reusable. I understand though that yagni to be more reusable. Regarding PutOnMarkingDeque, I changed the name of the callback, I think this is much better name that what I came with. If you don't like TraceWrappableFrom, does TraceDependantsOf sound better? https://codereview.chromium.org/1815153002/diff/60001/include/v8.h#newcode5841 include/v8.h:5841: * This function removes the tracer installed by `SetEmbedderHeapTracer` I will gladly remove the method, I don't think it will ever be used in practice. I added it only to go with the flow with other GC callbacks (where Remove methods are actually useful). I was thinking about introducing NoopEmpedderHeapTracer and setting it as a default in an isolate, so we don't have to nullcheck each time we access tracer. But as the code is not too bad with the nullchecks, I haven't done so yet. WDYT?
https://codereview.chromium.org/1815153002/diff/80001/include/v8.h File include/v8.h (right): https://codereview.chromium.org/1815153002/diff/80001/include/v8.h#newcode5373 include/v8.h:5373: GCPutOnMarkingDequeCallback callback); as discussed offline, I'd propose to not add callbacks we don't need yet
I gave it a week and moved further with the implementation so now the api is a little bit more stable. This cl only introduces the api, after landing this I'll upload the cl so v8 will actually use the api (if flag is present and tracer is set). I'm not sure about WrapperHiddenFields, as currently the knowledge that 0th hidden field is wrapper info and 1st field is the wrappable comes from the blink (or other embedders) and v8 shouldn't know or care. But in next cl you'll see that I collect the hidden fields in a list during incremental marking and process it all at once at the end. Holding a list of lists of hidden fields feels ugly, blink doesn't need the extra fields, and we might save few bits of memory.
Description was changed from ========== Introduce GCAfterMarkCompactCallback and the api for registering BUG= LOG=no ========== to ========== Introduce EmbedderHeapTracer BUG=468240 LOG=no ==========
https://codereview.chromium.org/1815153002/diff/100001/include/v8.h File include/v8.h (right): https://codereview.chromium.org/1815153002/diff/100001/include/v8.h#newcode5368 include/v8.h:5368: struct WrapperHiddenFields { why not just std::pair<void*,void*>? https://codereview.chromium.org/1815153002/diff/100001/include/v8.h#newcode5391 include/v8.h:5391: virtual void TraceRoots(Isolate* isolate) {} = 0; also add an empty line after this one https://codereview.chromium.org/1815153002/diff/100001/include/v8.h#newcode5399 include/v8.h:5399: WrapperHiddenFields internal_fields) {} same here. why not pass a const std::vector<HiddenFields>& ? https://codereview.chromium.org/1815153002/diff/100001/include/v8.h#newcode5404 include/v8.h:5404: virtual void ClearTracingMarks(Isolate* isolate) {} = 0; https://codereview.chromium.org/1815153002/diff/100001/include/v8.h#newcode5407 include/v8.h:5407: virtual ~EmbedderHeapTracer() {} = default; https://codereview.chromium.org/1815153002/diff/100001/include/v8.h#newcode5641 include/v8.h:5641: void AddObjectToMarkingDeque(PersistentBase<Object>* handle); why not make this a member of PersistentBase? https://codereview.chromium.org/1815153002/diff/100001/include/v8.h#newcode5893 include/v8.h:5893: EmbedderHeapTracer* embedder_heap_tracer(); shouldn't be needed https://codereview.chromium.org/1815153002/diff/100001/src/api.cc File src/api.cc (right): https://codereview.chromium.org/1815153002/diff/100001/src/api.cc#newcode7254 src/api.cc:7254: internal_isolate->heap()->AddObjectToMarkingDeque(object); we usually use i:: and i_ instead of internal. I'd also move this to global-handles.cc
https://codereview.chromium.org/1815153002/diff/100001/src/heap/heap.cc File src/heap/heap.cc (right): https://codereview.chromium.org/1815153002/diff/100001/src/heap/heap.cc#newco... src/heap/heap.cc:5427: IncrementalMarking::MarkObject(isolate()->heap(), heap_object); isolate()->heap() === this https://codereview.chromium.org/1815153002/diff/100001/src/heap/heap.cc#newco... src/heap/heap.cc:5595: DCHECK(tracer != nullptr); DCHECK_NOT_NULL() I'd also add CHECK_NULL(embedder_heap_tracer_)
Patchset #8 (id:140001) has been deleted
Patchset #7 (id:120001) has been deleted
Incorporated Jochen's wonderful comments https://codereview.chromium.org/1815153002/diff/100001/include/v8.h File include/v8.h (right): https://codereview.chromium.org/1815153002/diff/100001/include/v8.h#newcode5368 include/v8.h:5368: struct WrapperHiddenFields { Because I didn't know it exists :) Done. https://codereview.chromium.org/1815153002/diff/100001/include/v8.h#newcode5391 include/v8.h:5391: virtual void TraceRoots(Isolate* isolate) {} done https://codereview.chromium.org/1815153002/diff/100001/include/v8.h#newcode5399 include/v8.h:5399: WrapperHiddenFields internal_fields) {} Done https://codereview.chromium.org/1815153002/diff/100001/include/v8.h#newcode5404 include/v8.h:5404: virtual void ClearTracingMarks(Isolate* isolate) {} Done https://codereview.chromium.org/1815153002/diff/100001/include/v8.h#newcode5407 include/v8.h:5407: virtual ~EmbedderHeapTracer() {} Done https://codereview.chromium.org/1815153002/diff/100001/include/v8.h#newcode5641 include/v8.h:5641: void AddObjectToMarkingDeque(PersistentBase<Object>* handle); Done https://codereview.chromium.org/1815153002/diff/100001/include/v8.h#newcode5893 include/v8.h:5893: EmbedderHeapTracer* embedder_heap_tracer(); Sure it's not :) Removed. https://codereview.chromium.org/1815153002/diff/100001/src/api.cc File src/api.cc (right): https://codereview.chromium.org/1815153002/diff/100001/src/api.cc#newcode7254 src/api.cc:7254: internal_isolate->heap()->AddObjectToMarkingDeque(object); Done. https://codereview.chromium.org/1815153002/diff/100001/src/heap/heap.cc File src/heap/heap.cc (right): https://codereview.chromium.org/1815153002/diff/100001/src/heap/heap.cc#newco... src/heap/heap.cc:5427: IncrementalMarking::MarkObject(isolate()->heap(), heap_object); In the meantime I fixed a bug, ptal at new version https://codereview.chromium.org/1815153002/diff/100001/src/heap/heap.cc#newco... src/heap/heap.cc:5595: DCHECK(tracer != nullptr); Done.
https://codereview.chromium.org/1815153002/diff/160001/include/v8.h File include/v8.h (right): https://codereview.chromium.org/1815153002/diff/160001/include/v8.h#newcode603 include/v8.h:603: V8_INLINE void AddToMarkingDeque(Isolate* isolate); I do not think we should expose the notion of a marking deque over the API. We could give this method a more generic name like RegisterExternallyReferencedObject. https://codereview.chromium.org/1815153002/diff/160001/src/api.cc File src/api.cc (right): https://codereview.chromium.org/1815153002/diff/160001/src/api.cc#newcode681 src/api.cc:681: i::GlobalHandles::AddObjectToMarkingDeque(object, isolate); Why do you go through GlobalHandles? You could call the registration function on heap right away. https://codereview.chromium.org/1815153002/diff/160001/src/heap/heap.cc File src/heap/heap.cc (right): https://codereview.chromium.org/1815153002/diff/160001/src/heap/heap.cc#newco... src/heap/heap.cc:5425: DCHECK(mark_compact_collector()->in_use()); Add a DCHECK that checks if the object is on our heap. https://codereview.chromium.org/1815153002/diff/160001/src/heap/heap.h File src/heap/heap.h (right): https://codereview.chromium.org/1815153002/diff/160001/src/heap/heap.h#newcod... src/heap/heap.h:608: void AddObjectToMarkingDeque(Object** object); Again, API name change. Moreover, since you are already here can you create a proper // =========== // API. ====== // =========== section and add the methods called from the API to that section?
https://codereview.chromium.org/1815153002/diff/160001/include/v8.h File include/v8.h (right): https://codereview.chromium.org/1815153002/diff/160001/include/v8.h#newcode603 include/v8.h:603: V8_INLINE void AddToMarkingDeque(Isolate* isolate); Agree, done. https://codereview.chromium.org/1815153002/diff/160001/src/api.cc File src/api.cc (right): https://codereview.chromium.org/1815153002/diff/160001/src/api.cc#newcode681 src/api.cc:681: i::GlobalHandles::AddObjectToMarkingDeque(object, isolate); Seemed like I should leave a breadcrumb in the global handles, but I agree, it's more of a bloat. Removed. https://codereview.chromium.org/1815153002/diff/160001/src/heap/heap.cc File src/heap/heap.cc (right): https://codereview.chromium.org/1815153002/diff/160001/src/heap/heap.cc#newco... src/heap/heap.cc:5425: DCHECK(mark_compact_collector()->in_use()); Done https://codereview.chromium.org/1815153002/diff/160001/src/heap/heap.h File src/heap/heap.h (right): https://codereview.chromium.org/1815153002/diff/160001/src/heap/heap.h#newcod... src/heap/heap.h:608: void AddObjectToMarkingDeque(Object** object); Done
https://codereview.chromium.org/1815153002/diff/180001/include/v8.h File include/v8.h (right): https://codereview.chromium.org/1815153002/diff/180001/include/v8.h#newcode603 include/v8.h:603: V8_INLINE void RegisterExternalReference(Isolate* isolate); RegisterExternallyReferencedObject https://codereview.chromium.org/1815153002/diff/180001/src/heap/heap.h File src/heap/heap.h (right): https://codereview.chromium.org/1815153002/diff/180001/src/heap/heap.h#newcod... src/heap/heap.h:935: EmbedderHeapTracer* embedder_heap_tracer() { return embedder_heap_tracer_; } Move this one to the API section. Can you add other heap methods called from the API on a follow up CL? As a heap.h cleanup?
lgtm
lgtm
The CQ bit was checked by hlopko@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1815153002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1815153002/180001
Message was sent while issue was closed.
Description was changed from ========== Introduce EmbedderHeapTracer BUG=468240 LOG=no ========== to ========== Introduce EmbedderHeapTracer BUG=468240 LOG=no ==========
Message was sent while issue was closed.
Committed patchset #8 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== Introduce EmbedderHeapTracer BUG=468240 LOG=no ========== to ========== Introduce EmbedderHeapTracer BUG=468240 LOG=no Committed: https://crrev.com/289f3824973714e8a917bdcd485fcd8cbb58fdfd Cr-Commit-Position: refs/heads/master@{#35162} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/289f3824973714e8a917bdcd485fcd8cbb58fdfd Cr-Commit-Position: refs/heads/master@{#35162} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
