Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1289603002:
Put V8 extras into the snapshot (Closed)
DescriptionPut V8 extras into the snapshot
Previously, all extras were "experimental" and left out of the snapshot. This
patch moves them to the snapshot, so now all extras are non-experimental. A
future patch will re-introduce experimental extras as part of the linked bug.
R=yangguo@chromium.org
BUG=https://code.google.com/p/chromium/issues/detail?id=507137
LOG=Y
Committed: https://crrev.com/46d342523e39173b064870d89c68f6b90b0175fb
Cr-Commit-Position: refs/heads/master@{#30183}
Patch Set 1 #Patch Set 2 : Thin context does not get extras #Patch Set 3 : Get rid of testing stuff. All works. #
Total comments: 2
Patch Set 4 : Fix comment #
Total comments: 5
Patch Set 5 : Address review comments #Patch Set 6 : Trying to make gcc happier with more inline keywords #Patch Set 7 : Try appeasing gcc more. #msvc4lyfe #Patch Set 8 : Fix comment #
Total comments: 1
Patch Set 9 : Fix nit #
Total comments: 1
Messages
Total messages: 39 (17 generated)
domenic@chromium.org changed reviewers: + yangguo@chromium.org
A couple areas I wasn't as sure about: https://codereview.chromium.org/1289603002/diff/40001/src/bootstrapper.cc File src/bootstrapper.cc (right): https://codereview.chromium.org/1289603002/diff/40001/src/bootstrapper.cc#new... src/bootstrapper.cc:2611: if (context_type == THIN_CONTEXT) { This maybe should be != FULL_CONTEXT? https://codereview.chromium.org/1289603002/diff/40001/src/snapshot/serialize.h File src/snapshot/serialize.h (right): https://codereview.chromium.org/1289603002/diff/40001/src/snapshot/serialize.... src/snapshot/serialize.h:406: static const int kExtraNativesStringResource = 0x5e; Wasn't sure if there was a pattern here that was supposed to be followed so I just took an unused code...
The CQ bit was checked by domenic@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1289603002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1289603002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No L-G-T-M from a valid reviewer yet. Only full committers are accepted. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
LGTM, just a bunch of suggestions. https://codereview.chromium.org/1289603002/diff/60001/src/api.cc File src/api.cc (right): https://codereview.chromium.org/1289603002/diff/60001/src/api.cc#newcode397 src/api.cc:397: } I don't think any of this is actually necessary. What happens if you remove this and the loop above for Natives? https://codereview.chromium.org/1289603002/diff/60001/src/bootstrapper.cc File src/bootstrapper.cc (right): https://codereview.chromium.org/1289603002/diff/60001/src/bootstrapper.cc#new... src/bootstrapper.cc:3231: if (context_type != THIN_CONTEXT) { I suggest putting InstallExtraNatives here. Then you don't need to pass the context_type. https://codereview.chromium.org/1289603002/diff/60001/src/snapshot/serialize.cc File src/snapshot/serialize.cc (right): https://codereview.chromium.org/1289603002/diff/60001/src/snapshot/serialize.... src/snapshot/serialize.cc:581: Object* source = isolate_->heap()->extra_natives_source_cache()->get(i); now that we have this code three times, can we templatize it to reduce duplicate code? We have those GetCache templates in bootstrapper.cc, which we could move to natives.h/natives.cc as methods of NativesCollection<>. We can then use them here. https://codereview.chromium.org/1289603002/diff/60001/src/snapshot/serialize.... src/snapshot/serialize.cc:2208: serializer_->isolate()->heap()->extra_natives_source_cache(), We can then also replace the third argument with ExtraNatives::GetCache(). https://codereview.chromium.org/1289603002/diff/60001/src/snapshot/serialize.h File src/snapshot/serialize.h (right): https://codereview.chromium.org/1289603002/diff/60001/src/snapshot/serialize.... src/snapshot/serialize.h:406: static const int kExtraNativesStringResource = 0x5e; This is fine, but I'd group them a bit differently. Maybe 0x5d..0x5f for the three k*StringResources, and sort it to right after kDeferred, swap kVariableRepeat to 0x37 and leave 0x77 unused. Not that it really matters, but it would be somewhat nicer.
The CQ bit was checked by domenic@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1289603002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1289603002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel/builds/8615) v8_mac_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel/builds/8640)
The CQ bit was checked by domenic@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1289603002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1289603002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_avx2_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel/builds/...)
The CQ bit was checked by domenic@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1289603002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1289603002/120001
The CQ bit was unchecked by domenic@chromium.org
The CQ bit was checked by domenic@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1289603002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1289603002/140001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2015/08/13 at 12:28:24, yangguo wrote: > LGTM, just a bunch of suggestions. All suggestions implemented, including removing the loop(s) in api.cc (which indeed seem to be useless). Would appreciate a second review given the changes.
The CQ bit was checked by domenic@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1289603002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1289603002/160001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_avx2_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel/builds/...) v8_linux64_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel/builds/8629) v8_linux_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel/builds/8538)
Patchset #9 (id:160001) has been deleted
lgtm with small nit https://codereview.chromium.org/1289603002/diff/140001/src/snapshot/serialize.h File src/snapshot/serialize.h (right): https://codereview.chromium.org/1289603002/diff/140001/src/snapshot/serialize... src/snapshot/serialize.h:404: static const int kVariableRepeat = 0x37; small nit: order it before kVariableRawData please.
The CQ bit was checked by domenic@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yangguo@chromium.org Link to the patchset: https://codereview.chromium.org/1289603002/#ps170001 (title: "Fix nit")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1289603002/170001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1289603002/170001
Message was sent while issue was closed.
Committed patchset #9 (id:170001)
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/46d342523e39173b064870d89c68f6b90b0175fb Cr-Commit-Position: refs/heads/master@{#30183}
Message was sent while issue was closed.
mstarzinger@chromium.org changed reviewers: + mstarzinger@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/1289603002/diff/170001/src/snapshot/natives.h File src/snapshot/natives.h (right): https://codereview.chromium.org/1289603002/diff/170001/src/snapshot/natives.h... src/snapshot/natives.h:9: #include "src/heap/heap-inl.h" I have been working the entire last week to get rid of these includes. There is "tools/check-inline-includes.sh" which warns about these kind of includes (i.e. foo-inl.h included in bar.h) and I was down to 5 failures, now we are back to 6. So this is somewhat disappointing at this point. :(
Message was sent while issue was closed.
On 2015/08/17 at 16:47:26, mstarzinger wrote: > https://codereview.chromium.org/1289603002/diff/170001/src/snapshot/natives.h > File src/snapshot/natives.h (right): > > https://codereview.chromium.org/1289603002/diff/170001/src/snapshot/natives.h... > src/snapshot/natives.h:9: #include "src/heap/heap-inl.h" > I have been working the entire last week to get rid of these includes. There is "tools/check-inline-includes.sh" which warns about these kind of includes (i.e. foo-inl.h included in bar.h) and I was down to 5 failures, now we are back to 6. So this is somewhat disappointing at this point. :( Yes, I saw your work and am sorry for making it harder :(. I tried without the includes first actually, and it compiled great on Windows, but not on the build bots. The whole situation is a mess and I am hopeful you can help straighten it out!!
Message was sent while issue was closed.
On 2015/08/17 17:51:27, domenic wrote: > On 2015/08/17 at 16:47:26, mstarzinger wrote: > > https://codereview.chromium.org/1289603002/diff/170001/src/snapshot/natives.h > > File src/snapshot/natives.h (right): > > > > > https://codereview.chromium.org/1289603002/diff/170001/src/snapshot/natives.h... > > src/snapshot/natives.h:9: #include "src/heap/heap-inl.h" > > I have been working the entire last week to get rid of these includes. There > is "tools/check-inline-includes.sh" which warns about these kind of includes > (i.e. foo-inl.h included in bar.h) and I was down to 5 failures, now we are back > to 6. So this is somewhat disappointing at this point. :( > > Yes, I saw your work and am sorry for making it harder :(. I tried without the > includes first actually, and it compiled great on Windows, but not on the build > bots. The whole situation is a mess and I am hopeful you can help straighten it > out!! Fix for the heap-inl.h thing: https://codereview.chromium.org/1292533005/ |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
