Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2342133003:
Don't force eager parsing when natives are allowed. (Closed)
DescriptionDon't force eager parsing when natives are allowed.
PreParser is already capable of handling natives, because
ParseV8Intrinsic was moved to ParserBase. There's no reason to force
eager parsing when natives are allowed.
R=nikolaos@chromium.org, mstarzinger@chromium.org
BUG=v8:5398
Committed: https://crrev.com/f7fadf268c097f4f0c1cc105b91336d876e3c7ba
Cr-Commit-Position: refs/heads/master@{#39501}
Patch Set 1 #Patch Set 2 : ... but eager parse native scripts #Patch Set 3 : cleaner #Messages
Total messages: 33 (19 generated)
The CQ bit was checked by marja@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
ptal
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Other than the title, which should be (I think) "Don't force eager compilation...", I think it looks OK. To be honest, I have no idea what effects this will have, so I suggest that somebody who understands it better gives a LGTM. (I know this will count as one... :-) )
Description was changed from ========== Don't force lazy compilation when natives are allowed. PreParser is already capable of handling natives, because ParseV8Intrinsic was moved to ParserBase. There's no reason to force eager parsing when natives are allowed. R=nikolaos@chromium.org, mstarzinger@chromium.org BUG=v8:5398 ========== to ========== Don't force eager compilation when natives are allowed. PreParser is already capable of handling natives, because ParseV8Intrinsic was moved to ParserBase. There's no reason to force eager parsing when natives are allowed. R=nikolaos@chromium.org, mstarzinger@chromium.org BUG=v8:5398 ==========
Description was changed from ========== Don't force eager compilation when natives are allowed. PreParser is already capable of handling natives, because ParseV8Intrinsic was moved to ParserBase. There's no reason to force eager parsing when natives are allowed. R=nikolaos@chromium.org, mstarzinger@chromium.org BUG=v8:5398 ========== to ========== Don't force eager parsing when natives are allowed. PreParser is already capable of handling natives, because ParseV8Intrinsic was moved to ParserBase. There's no reason to force eager parsing when natives are allowed. R=nikolaos@chromium.org, mstarzinger@chromium.org BUG=v8:5398 ==========
marja@chromium.org changed reviewers: + adamk@chromium.org
oops, yes, the title was the wrong way around :) adamk, could you have a look too? My guess is that the only reason for not lazy parsing when natives were allowed was that ParseV8Intrinsic was not implemented for PreParser back in the day when Parser and PreParser were separate. And it was not implemented because there was no need (PreParser is just a hac... an optimization).
adamk@chromium.org changed reviewers: + yangguo@chromium.org
I was under the impression that this behavior existed to make sure that the context heap snapshot we ship with V8 included as much code as possible. But I don't know if I actually read that anywhere or just assumed it. Yang should know, though.
marja@chromium.org changed reviewers: + vogelheim@chromium.org - yangguo@chromium.org
yangguo@ is on a leave; substituting w/ vogelheim@ to comment the snapshot aspect.
... maybe we can force eager compilation for the needed files (containing the native function implementations) some other way. Then we could enable lazy parsing for test files which only *use* native functions.
Despite the green try jobs, this CL is wrong: we indeed need a more clever mechanism when building native scripts. (It's unfortunate that allow_natives means both the definition and the usage.) I'll fix this and ping this CL again.
The CQ bit was checked by marja@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by marja@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
vogelheim@, ptal Ok, now I think I got this under control. This CL cleans up the laziness decision, now it's done once and for all, so not all places need to check is_native() and extension_ and FLAG_lazy etc.
(Also, I found out that is_native() is the thing we should be checking to get native scripts parsed eagerly...)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux_chromium_gn_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_chromium_gn_rel/bu...)
lgtm
The CQ bit was checked by marja@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nikolaos@chromium.org Link to the patchset: https://codereview.chromium.org/2342133003/#ps40001 (title: "cleaner")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Don't force eager parsing when natives are allowed. PreParser is already capable of handling natives, because ParseV8Intrinsic was moved to ParserBase. There's no reason to force eager parsing when natives are allowed. R=nikolaos@chromium.org, mstarzinger@chromium.org BUG=v8:5398 ========== to ========== Don't force eager parsing when natives are allowed. PreParser is already capable of handling natives, because ParseV8Intrinsic was moved to ParserBase. There's no reason to force eager parsing when natives are allowed. R=nikolaos@chromium.org, mstarzinger@chromium.org BUG=v8:5398 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Don't force eager parsing when natives are allowed. PreParser is already capable of handling natives, because ParseV8Intrinsic was moved to ParserBase. There's no reason to force eager parsing when natives are allowed. R=nikolaos@chromium.org, mstarzinger@chromium.org BUG=v8:5398 ========== to ========== Don't force eager parsing when natives are allowed. PreParser is already capable of handling natives, because ParseV8Intrinsic was moved to ParserBase. There's no reason to force eager parsing when natives are allowed. R=nikolaos@chromium.org, mstarzinger@chromium.org BUG=v8:5398 Committed: https://crrev.com/f7fadf268c097f4f0c1cc105b91336d876e3c7ba Cr-Commit-Position: refs/heads/master@{#39501} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/f7fadf268c097f4f0c1cc105b91336d876e3c7ba Cr-Commit-Position: refs/heads/master@{#39501}
Message was sent while issue was closed.
On 2016/09/19 11:47:08, commit-bot: I haz the power wrote: > Patchset 3 (id:??) landed as > https://crrev.com/f7fadf268c097f4f0c1cc105b91336d876e3c7ba > Cr-Commit-Position: refs/heads/master@{#39501} To answer the question above. I don't know why lazy parsing is not allowed for natives. I faintly remember it somehow related to parsing natives syntax, but that may long be out of date. It's definitely not done for the snapshot, since we generally don't include any compiled JS code in the snapshot at all. |
