Hej,
Thanks for highlighting hickups and taking the effort to improve on this!
Let's
leave the refactoring in a separate class for another issue.
Can you share a bit more on the bottleneck in node.js when this code is
involved? Any workloads or benchmarks you are tracking?
In any case, if tracking the buffers results in a performance issue we
should
definitely take the effort of having a closer look. In general, the
XXXHelper()
functions don't make much sense as they are either one liners, or hide
additional loops.
The FreeDeadArrayBuffers() method is in particular messy. During inlining I
found a way to simplify this a lot, in the end resulting only in a single
loop
over not-yet-discovered-buffers (which is a different map depending on the
caller).
I think we should go this way as otherwise the logic is too complicate to
see
what's going on.
https://codereview.chromium.org/1316873004/diff/1/src/heap/heap.cc
File src/heap/heap.cc (right):
https://codereview.chromium.org/1316873004/diff/1/src/heap/heap.cc#newcode1739
src/heap/heap.cc:1739: void
Heap::RegisterNewArrayBufferHelper(std::map<void*, size_t>&
live_buffers,
Please get rid of this function and inline it into
RegisterNewArrayBuffer.
https://codereview.chromium.org/1316873004/diff/1/src/heap/heap.cc#newcode1745
src/heap/heap.cc:1745: void Heap::UnregisterArrayBufferHelper(
Ditto, please inline the call manually.
https://codereview.chromium.org/1316873004/diff/1/src/heap/heap.cc#newcode1754
src/heap/heap.cc:1754: void Heap::RegisterLiveArrayBufferHelper(
Please get rid of this function and inline it into
RegisterLiveArrayBuffer.
https://codereview.chromium.org/1316873004/diff/1/src/heap/heap.cc#newcode1760
src/heap/heap.cc:1760: size_t Heap::FreeDeadArrayBuffersHelper(
We should get rid of this one. See below.
https://codereview.chromium.org/1316873004/diff/1/src/heap/heap.cc#newcode1784
src/heap/heap.cc:1784: void Heap::TearDownArrayBuffersHelper(
Also inline please.
https://codereview.chromium.org/1316873004/diff/1/src/heap/heap.cc#newcode1834
src/heap/heap.cc:1834: void Heap::FreeDeadArrayBuffers(bool
from_scavenge) {
The logic here is way too complicated as the semantic is to maintain two
distinct sets for array buffers.
I inlined, reshuffled and consolidated the calls. I think we should get
rid of the helper method as the actual logic seems to be pretty simple.
Please check the simplification. I am using .erase() with buffers (there
exists an overload that takes a map). The loops (over maps) get unfolded
into a single loop in the last step.
======== Step 0 (baseline; inlined FreeDeadArrayBuffersHelper call from
patch) ========
FreeDeadArrayBuffers(from_scavenge):
if from_scavenge:
not_yet_discovered_array_buffers_.erase(not_yet_discovered_array_buffers_for_scavenge_)
else:
live_array_buffers_for_scavenge_.erase(not_yet_discovered_array_buffers_)
# inlined FreeDeadArrayBuffersHelper
# parameters are unfolded here
if from_scavenge:
not_yet_discovered_buffers =
not_yet_discovered_array_buffers_for_scavenge_
else:
not_yet_discovered_buffers = not_yet_discovered_array_buffers_
for buffer in not_yet_discovered_buffers:
isolate.free_array_buffer(buffer)
if (!from_scavenge):
live_array_buffers_.erase(buffer)
live_array_buffers_for_scavenge_.erase(buffer)
if (from_scavenge):
not_yet_discovered_buffers = live_array_buffers_for_scavenge_;
else:
not_yet_discovered_buffers = live_array_buffers_;
not_yet_discovered_buffers.insert(live_array_buffers_for_scavenge.begin(),
...end())
======== Step 1 (shuffle inlined stuff around) ========
FreeDeadArrayBuffers(from_scavenge):
if from_scavenge:
not_yet_discovered_array_buffers_.erase(not_yet_discovered_array_buffers_for_scavenge_)
not_yet_discovered_buffers =
not_yet_discovered_array_buffers_for_scavenge_
live_array_buffers_for_scavenge_.erase(not_yet_discovered_buffers)
else:
live_array_buffers_for_scavenge_.erase(not_yet_discovered_array_buffers_)
not_yet_discovered_buffers = not_yet_discovered_array_buffers_
live_array_buffers_for_scavenge_.erase(not_yet_discovered_buffers)
live_array_buffers_.erase(not_yet_discovered_buffers)
# rest of inlined FreeDeadArrayBuffersHelper
for buffer in not_yet_discovered_buffers:
isolate.free_array_buffer(buffer)
if (from_scavenge):
not_yet_discovered_buffers = live_array_buffers_for_scavenge_;
else:
not_yet_discovered_buffers = live_array_buffers_;
not_yet_discovered_buffers.insert(live_array_buffers_for_scavenge_)
======== Step 2 (re-orde / remove duplicates) ========
FreeDeadArrayBuffers(from_scavenge):
if from_scavenge:
tmp_not_yet_discovered_buffers =
not_yet_discovered_array_buffers_for_scavenge_
not_yet_discovered_array_buffers_.erase((tmp_not_yet_discovered_buffers)
live_array_buffers_for_scavenge_.erase(tmp_not_yet_discovered_buffers)
else:
tmp_not_yet_discovered_buffers = not_yet_discovered_array_buffers_
live_array_buffers_for_scavenge_.erase(tmp_not_yet_discovered_buffers)
live_array_buffers_.erase(tmp_not_yet_discovered_buffers)
# rest of inlined FreeDeadArrayBuffersHelper
for buffer in tmp_not_yet_discovered_buffers:
isolate.free_array_buffer(buffer)
if (from_scavenge):
not_yet_discovered_buffers = live_array_buffers_for_scavenge_;
else:
not_yet_discovered_buffers = live_array_buffers_;
not_yet_discovered_buffers.insert(live_array_buffers_for_scavenge_)
======== Step 3 (consolidate into a single loop) ========
FreeDeadArrayBuffers(from_scavenge):
if from_scavenge:
tmp_not_yet_discovered_buffers =
not_yet_discovered_array_buffers_for_scavenge_
else:
tmp_not_yet_discovered_buffers = not_yet_discovered_array_buffers_
for buffer in tmp_not_yet_discovered_buffers:
isolate.free_array_buffer(buffer)
live_array_buffers_for_scavenge_.erase(buffer)
if (from_scavenge):
not_yet_discovered_array_buffers_.erase(buffer)
else:
live_array_buffers_.erase(tmp_not_yet_discovered_buffers)
if (from_scavenge):
not_yet_discovered_buffers = live_array_buffers_for_scavenge_;
else:
not_yet_discovered_buffers = live_array_buffers_;
not_yet_discovered_buffers.insert(live_array_buffers_for_scavenge_)
https://codereview.chromium.org/1316873004/
--
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/d/optout.
