Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2240513003:
Scanner::LiteralBuffer usage cleanup. (Closed)
DescriptionScanner::LiteralBuffer usage cleanup.
1, restrict use of LiteralBuffers to the tokens that actually need it.
- E.g., previously the Token::FUNCTION would have a literal buffer
containing "function", which was never actually used.
- This eliminates copies of the string data for every call to
PeekAhead or SetBookmark.
2, document & enforce the "secret" Scanner API contract w/ DCHECK
- Document & check the correspondence of token value and literal buffer.
- Document & check preconditions for calling PeekAhead, ScanRegExp*,
ScanTemplate*.
BUG=v8:4947
Committed: https://crrev.com/c677f813814f5ead481fc2c91f7834989abccd25
Cr-Commit-Position: refs/heads/master@{#38677}
Patch Set 1 #Patch Set 2 : Fix handling of surrogates that are really ASCII. #Patch Set 3 : Avoid int conversion issues w/ some compilers. #Patch Set 4 : Avoid more int conversion & compiler issues w/ some other compilers. #
Total comments: 4
Patch Set 5 : Feedback fix compiler issues. #
Messages
Total messages: 53 (45 generated)
The CQ bit was checked by vogelheim@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 unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_asan_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng/buil...) v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...) v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/10917) v8_linux_arm64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng/build...) v8_linux_arm_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng/builds/...) v8_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/10909) v8_linux_mips64el_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...) v8_linux_mipsel_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...) v8_linux_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/10856)
Description was changed from
==========
Scanner::LiteralBuffer usage cleanup.
1, restrict use of LiteralBuffers to the tokens that actually need it.
- E.g., previously the Token::FUNCTION would have a literal buffer
containing "function", which was never actually used.
- This eliminateds copies of the string data for every call to
PeekAhead or SetBookmark.
2, document & enforce the "secret" Scanner API contract w/ DCHECK
- Document & check the correspondence of token value and literal buffer.
- Document & check preconditions for calling PeekAhead, ScanRegExp*,
ScanTemplate*.
BUG=v8:4947
==========
to
==========
Scanner::LiteralBuffer usage cleanup.
1, restrict use of LiteralBuffers to the tokens that actually need it.
- E.g., previously the Token::FUNCTION would have a literal buffer
containing "function", which was never actually used.
- This eliminates copies of the string data for every call to
PeekAhead or SetBookmark.
2, document & enforce the "secret" Scanner API contract w/ DCHECK
- Document & check the correspondence of token value and literal buffer.
- Document & check preconditions for calling PeekAhead, ScanRegExp*,
ScanTemplate*.
BUG=v8:4947
==========
Patchset #1 (id:1) has been deleted
The CQ bit was checked by vogelheim@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 unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_asan_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng/buil...) v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)
The CQ bit was checked by vogelheim@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 unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_asan_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng/buil...)
The CQ bit was checked by vogelheim@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 unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_win_compile_dbg on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_compile_dbg/builds/2...)
The CQ bit was checked by vogelheim@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 unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_win_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng/builds/12457)
vogelheim@chromium.org changed reviewers: + marja@chromium.org
ptal. (I still need to fix the Win compiler issue w/ constexpr.)
lgtm I guess we used to have literals for "function" etc since we just collected them before knowing it's a keyword. But ofc copying (after we already know) is then unnecessary overhead. Does this CL already improve our code load score? https://codereview.chromium.org/2240513003/diff/80001/src/parsing/scanner.cc File src/parsing/scanner.cc (left): https://codereview.chromium.org/2240513003/diff/80001/src/parsing/scanner.cc#... src/parsing/scanner.cc:1247: if (escaped) { \ This was dead code, right? https://codereview.chromium.org/2240513003/diff/80001/src/parsing/scanner.cc File src/parsing/scanner.cc (right): https://codereview.chromium.org/2240513003/diff/80001/src/parsing/scanner.cc#... src/parsing/scanner.cc:762: DCHECK(token.raw_literal_chars != nullptr); DCHECK_NOT_NULL?
https://codereview.chromium.org/2240513003/diff/80001/src/parsing/scanner.cc File src/parsing/scanner.cc (left): https://codereview.chromium.org/2240513003/diff/80001/src/parsing/scanner.cc#... src/parsing/scanner.cc:1247: if (escaped) { \ On 2016/08/16 07:20:42, marja wrote: > This was dead code, right? Nope, very undead. Moved into ScanIdentifierSuffix. This is triggered by the bool escaped, which was set 'false' at all call sites except one. So I just moved it to the one location that actually needs it, rather than have it macro-ified in a macro instantiated from ~5 places with all except one turning it off.
The CQ bit was checked by vogelheim@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 unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_win64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel_ng/builds/12532)
The CQ bit was checked by vogelheim@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 unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_win64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel_ng/builds/12538)
The CQ bit was checked by vogelheim@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 unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #5 (id:100001) has been deleted
Patchset #5 (id:120001) has been deleted
https://codereview.chromium.org/2240513003/diff/80001/src/parsing/scanner.cc File src/parsing/scanner.cc (right): https://codereview.chromium.org/2240513003/diff/80001/src/parsing/scanner.cc#... src/parsing/scanner.cc:762: DCHECK(token.raw_literal_chars != nullptr); On 2016/08/16 07:20:42, marja wrote: > DCHECK_NOT_NULL? Done.
On 2016/08/16 07:20:43, marja wrote: > lgtm > > I guess we used to have literals for "function" etc since we just collected them > before knowing it's a keyword. But ofc copying (after we already know) is then > unnecessary overhead. Does this CL already improve our code load score? +1% on one platform; -0.6% on another; both times the effect is less than the variance in scores.
The CQ bit was checked by vogelheim@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 unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by vogelheim@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from marja@chromium.org Link to the patchset: https://codereview.chromium.org/2240513003/#ps140001 (title: "Feedback fix compiler issues.")
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
==========
Scanner::LiteralBuffer usage cleanup.
1, restrict use of LiteralBuffers to the tokens that actually need it.
- E.g., previously the Token::FUNCTION would have a literal buffer
containing "function", which was never actually used.
- This eliminates copies of the string data for every call to
PeekAhead or SetBookmark.
2, document & enforce the "secret" Scanner API contract w/ DCHECK
- Document & check the correspondence of token value and literal buffer.
- Document & check preconditions for calling PeekAhead, ScanRegExp*,
ScanTemplate*.
BUG=v8:4947
==========
to
==========
Scanner::LiteralBuffer usage cleanup.
1, restrict use of LiteralBuffers to the tokens that actually need it.
- E.g., previously the Token::FUNCTION would have a literal buffer
containing "function", which was never actually used.
- This eliminates copies of the string data for every call to
PeekAhead or SetBookmark.
2, document & enforce the "secret" Scanner API contract w/ DCHECK
- Document & check the correspondence of token value and literal buffer.
- Document & check preconditions for calling PeekAhead, ScanRegExp*,
ScanTemplate*.
BUG=v8:4947
==========
Message was sent while issue was closed.
Committed patchset #5 (id:140001)
Message was sent while issue was closed.
Description was changed from
==========
Scanner::LiteralBuffer usage cleanup.
1, restrict use of LiteralBuffers to the tokens that actually need it.
- E.g., previously the Token::FUNCTION would have a literal buffer
containing "function", which was never actually used.
- This eliminates copies of the string data for every call to
PeekAhead or SetBookmark.
2, document & enforce the "secret" Scanner API contract w/ DCHECK
- Document & check the correspondence of token value and literal buffer.
- Document & check preconditions for calling PeekAhead, ScanRegExp*,
ScanTemplate*.
BUG=v8:4947
==========
to
==========
Scanner::LiteralBuffer usage cleanup.
1, restrict use of LiteralBuffers to the tokens that actually need it.
- E.g., previously the Token::FUNCTION would have a literal buffer
containing "function", which was never actually used.
- This eliminates copies of the string data for every call to
PeekAhead or SetBookmark.
2, document & enforce the "secret" Scanner API contract w/ DCHECK
- Document & check the correspondence of token value and literal buffer.
- Document & check preconditions for calling PeekAhead, ScanRegExp*,
ScanTemplate*.
BUG=v8:4947
Committed: https://crrev.com/c677f813814f5ead481fc2c91f7834989abccd25
Cr-Commit-Position: refs/heads/master@{#38677}
==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/c677f813814f5ead481fc2c91f7834989abccd25 Cr-Commit-Position: refs/heads/master@{#38677} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
