Committed patchset #8 (id:140001)
https://codereview.chromium.org/1316873004/
--
--
v8-dev mailing list
v8-dev@googlegroups.com
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
Patchset 8 (id:??) landed as
https://crrev.com/9e3676da9ab1aaf7de3e8582cb3fdefcc3dbaf33
Cr-Commit-Position: refs/heads/master@{#30495}
https://codereview.chromium.org/1316873004/
--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
---
You received this
Michael,
I have figured out the problem. See my changes to mark-compact.cc.
The buffers from new space was evacuated after being freed. It should be
better
now.
Going to run tests locally and submit the thing to CQ again.
Thanks!
https://codereview.chromium.org/1316873004/
--
--
v8-dev
CQ is trying da patch. Follow status at
https://chromium-cq-status.appspot.com/patch-status/1316873004/140001
View timeline at
https://chromium-cq-status.appspot.com/patch-timeline/1316873004/140001
https://codereview.chromium.org/1316873004/
--
--
v8-dev mailing list
v8-dev@googlegroups.com
On 2015/09/01 09:01:36, Michael Lippautz wrote:
https://codereview.chromium.org/1316873004/diff/140001/src/heap/mark-compact.cc
File src/heap/mark-compact.cc (right):
https://codereview.chromium.org/1316873004/diff/140001/src/heap/mark-compact.cc#newcode4436
src/heap/mark-compact.cc:4436: //
A revert of this CL (patchset #8 id:140001) has been created in
https://codereview.chromium.org/1302233007/ by mlippa...@chromium.org.
The reason for reverting is: Precautionary revert. The change is
incomplete..
https://codereview.chromium.org/1316873004/
--
--
v8-dev mailing list
Please wait next time when doing a change in another component, even if
l-g-t-m-ed already. The order of invocations during GC is not something to
just
change.
Also, I feel we should be able to call FreeDeadArrayBuffers at any time
after
marking. Please see comment and elaborate a bit more
https://codereview.chromium.org/1316873004/diff/140001/src/heap/mark-compact.cc
File src/heap/mark-compact.cc (right):
https://codereview.chromium.org/1316873004/diff/140001/src/heap/mark-compact.cc#newcode4436
src/heap/mark-compact.cc:4436: // NOTE: ArrayBuffers must be evacuated
first, before
Ok, you are definitely right. Looking through the logs that I have
collected I
see the following scenario:
1. Marking starts
2. ArrayBuffers in NewSpace are visited and marked as live by removing them
from
not_yet_..._for_scavenge
3. Scavenge starts, PrepareArrayBufferDiscovery is called,
Sorry for pushing it out without your ack. I hope this patch looks ok, if
not -
please feel free to revert it.
https://codereview.chromium.org/1316873004/diff/40001/src/heap/heap.cc
File src/heap/heap.cc (right):
On 2015/09/01 09:15:46, fedor.indutny wrote:
Ok, you are definitely right. Looking through the logs that I have
collected I
see the following scenario:
1. Marking starts
2. ArrayBuffers in NewSpace are visited and marked as live by removing
them
from
not_yet_..._for_scavenge
3. Scavenge
On 2015/08/28 20:49:41, fedor.indutny wrote:
The results of benchmarks are:
./node-slow 1 && ./node-no-inline-fast 1 && ./node-fast 1
4997.4 ns/op
4701.6 ns/op
4685.7 ns/op
NOTE: `no-inline-fast` is the initial version of patch, `fast` - is
current,
`slow` - vanilla v8.
Please add the
lgtm with comments.
The next step would be moving the logic out of Heap into an
ArrayBufferTracker
that lives in heap/ and finally add some tests. It would probably have to
be a
cctest that checks the transitions from one map into another.
CQ is trying da patch. Follow status at
https://chromium-cq-status.appspot.com/patch-status/1316873004/60001
View timeline at
https://chromium-cq-status.appspot.com/patch-timeline/1316873004/60001
https://codereview.chromium.org/1316873004/
--
--
v8-dev mailing list
v8-dev@googlegroups.com
Try jobs failed on following builders:
v8_mac_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel/builds/9247)
v8_win_nosnap_shared_compile_rel on tryserver.v8 (JOB_FAILED,
On 2015/08/31 06:31:58, Michael Lippautz wrote:
On 2015/08/28 20:49:41, fedor.indutny wrote:
> The results of benchmarks are:
>
> ./node-slow 1 && ./node-no-inline-fast 1 && ./node-fast 1
> 4997.4 ns/op
> 4701.6 ns/op
> 4685.7 ns/op
>
> NOTE: `no-inline-fast` is the initial version of patch,
Made a typo in debug-only line. Going to rebuild it on my machine, and
resubmit
it again. Sorry about this!
(Also sounds like a good chance to update the description)
https://codereview.chromium.org/1316873004/
--
--
v8-dev mailing list
v8-dev@googlegroups.com
CQ is trying da patch. Follow status at
https://chromium-cq-status.appspot.com/patch-status/1316873004/80001
View timeline at
https://chromium-cq-status.appspot.com/patch-timeline/1316873004/80001
https://codereview.chromium.org/1316873004/
--
--
v8-dev mailing list
v8-dev@googlegroups.com
Try jobs failed on following builders:
v8_linux_nodcheck_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel/builds/6081)
https://codereview.chromium.org/1316873004/
--
--
v8-dev mailing list
v8-dev@googlegroups.com
Michael,
Looks like there is some issue on linux buildbot. I will investigate it, and
will try to submit a fix as soon as possible.
So far it looks like a double-free error to me.
https://codereview.chromium.org/1316873004/
--
--
v8-dev mailing list
v8-dev@googlegroups.com
On 2015/08/31 06:45:29, fedor.indutny wrote:
On 2015/08/31 06:31:58, Michael Lippautz wrote:
> On 2015/08/28 20:49:41, fedor.indutny wrote:
> > I guess making them a scavenges might be the next step to improve the
> > performance.
> >
>
> I don't think there's much to improve on here with the
On 2015/08/31 07:16:39, Michael Lippautz wrote:
On 2015/08/31 06:45:29, fedor.indutny wrote:
> On 2015/08/31 06:31:58, Michael Lippautz wrote:
> > On 2015/08/28 20:49:41, fedor.indutny wrote:
> > > I guess making them a scavenges might be the next step to improve
the
> > > performance.
> > >
Besides fixing the double free, please also rebase on master. We had to land
https://codereview.chromium.org/1325643002/ as a bugfix today.
https://codereview.chromium.org/1316873004/
--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
---
You received this
On 2015/08/31 16:08:14, Michael Lippautz wrote:
Besides fixing the double free, please also rebase on master. We had to
land
https://codereview.chromium.org/1325643002/ as a bugfix today.
Thanks for letting me know! I think that it has a bug too ;) Commented
there.
It looks like it might
On 2015/08/31 20:50:01, fedor.indutny wrote:
On 2015/08/31 16:08:14, Michael Lippautz wrote:
> Besides fixing the double free, please also rebase on master. We had to
land
> https://codereview.chromium.org/1325643002/ as a bugfix today.
Thanks for letting me know! I think that it has a bug
register new_space 0x1031335d0
register new_space 0x103560110
live inc 0x102381a30
live inc 0x102381970
live inc 0x103560110
live inc 0x1031335d0
PrepareArrayBufferDiscovery
live scavenge 0x1031335d0
live scavenge 0x103560110
enter adjust scavenge
leave adjust scavenge
[53385:0x102287910]
On 2015/08/31 07:21:23, fedor.indutny wrote:
On 2015/08/31 07:16:39, Michael Lippautz wrote:
> On 2015/08/31 06:45:29, fedor.indutny wrote:
> > On 2015/08/31 06:31:58, Michael Lippautz wrote:
> > > On 2015/08/28 20:49:41, fedor.indutny wrote:
> > All of the allocated buffers are in the new
Inlined everything, thank you again!
(Sorry, going to sleep now. Hope it looks good now ;) )
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:
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#newcode1784
src/heap/heap.cc:1784: void Heap::TearDownArrayBuffersHelper(
On 2015/08/28 08:05:02, Michael Lippautz wrote:
Also
On 2015/08/28 09:48:40, fedor.indutny wrote:
Hello again,
Thanks for the thorough review! Will fix everything mentioned as soon as
possible, hopefully today.
We see two bottlenecks in node.js: one happens during buffer allocation,
one
during GC. Both add together and make things slightly
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
Hello again,
Thanks for the thorough review! Will fix everything mentioned as soon as
possible, hopefully today.
We see two bottlenecks in node.js: one happens during buffer allocation, one
during GC. Both add together and make things slightly slower than they could
potentially be. Here is a
On 2015/08/28 10:02:37, fedor.indutny wrote:
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#newcode1784
src/heap/heap.cc:1784: void Heap::TearDownArrayBuffersHelper(
On
Fedor,
Getting there! Now we can actually see what's happening :)
The code will get much better (and I suspect much faster). The main idea is
that
we only process XXX_scavenge_ maps during scavenges and everything during
the
full GC.
Let me also note that V8’s new generation (scavenger)
The results of benchmarks are:
./node-slow 1 ./node-no-inline-fast 1 ./node-fast 1
4997.4 ns/op
4701.6 ns/op
4685.7 ns/op
NOTE: `no-inline-fast` is the initial version of patch, `fast` - is current,
`slow` - vanilla v8.
The other thing that I have noticed is that there are not much scavenges
Thank you, Michael!
I don't mind moving it into a separate class. But maybe I could do it in
separate CL?
https://codereview.chromium.org/1316873004/
--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
---
You received this message because you are
-svenpanne, -indutny, +hpayer
Generally fine;
How about factoring out the whole logic into an ArrayBufferTracker that is
tied
to heap or isolate? The logic has no dependency on heap and as far as I see
there are only calls into isolate. The only call from heap is to
FreeDeadArrayBuffers.
Reviewers: svenpanne, Michael Lippautz, indutny,
Message:
I'm no longer sure, which account was used to submit this CL. Emailing from
the
second one, just in case.
Description:
heap: make array buffer maps non-intersecting
Remove intersection from the `std::map`s representing current live
38 matches
Mail list logo
