Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1995453002:
[stubs] Extend HasProperty stub with dictionary-mode and double-elements objects support. (Closed)
|
Created:
4 years, 7 months ago by Igor Sheludko Modified:
4 years, 6 months ago Reviewers:
Toon Verwaest CC:
v8-reviews_googlegroups.com Base URL:
https://chromium.googlesource.com/v8/v8.git@master Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[stubs] Extend HasProperty stub with dictionary-mode, string wrapper and double-elements objects support.
This CL also replaces some Branch() usages with GotoIf/GotoUnless.
(This is a reland after fixing issues that prevented this CL from landing in other CLs).
BUG=v8:2743
LOG=Y
Committed: https://crrev.com/24066b6df4259b302edfa1db884c479008776a7e
Cr-Commit-Position: refs/heads/master@{#36657}
Committed: https://crrev.com/3c4f903e561d23bbec3b33767b51699be79f7b2d
Cr-Commit-Position: refs/heads/master@{#36686}
Patch Set 1 #Patch Set 2 : Fixed 32-bit platform issues #Patch Set 3 : Fixes and ArrayIndex-related cleanup in objects.h/.cc #
Total comments: 6
Patch Set 4 : Adding tests, fixing bugs and cleaning up stuff #Patch Set 5 : Rebasing and cleanup #
Total comments: 1
Patch Set 6 : Fix for GCC issue #Patch Set 7 : Rebasing #Patch Set 8 : Fix for GCC compilation issue #Patch Set 9 : Rebasing after extracting a couple of test infrastructure fixes to separate CLs #Patch Set 10 : Drive-by fix: replace Load()s with LoadObjectField()s #
Depends on Patchset: Messages
Total messages: 76 (42 generated)
Description was changed from ========== Extend HasProperty stub with dictionary mode objects support. BUG=v8:2743 LOG=Y ========== to ========== Extend HasProperty stub with dictionary-mode and double-elements objects support. This CL also replaces some Branch() usages with GotoIf/GotoUnless. BUG=v8:2743 LOG=Y ==========
The CQ bit was checked by ishell@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/1995453002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1995453002/1
ishell@chromium.org changed reviewers: + verwaest@chromium.org
PTAL
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux_arm_rel_ng on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng/builds/...) v8_linux_rel_ng on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/5861) v8_mac_rel_ng on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/1768)
Patchset #2 (id:20001) has been deleted
The CQ bit was checked by ishell@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/1995453002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1995453002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_mac_rel_ng on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/1772) v8_mac_rel_ng_triggered on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng_triggered/bui...)
Patchset #3 (id:60001) has been deleted
The CQ bit was checked by ishell@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/1995453002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1995453002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I updated the CL, now it works :)
lgtm. may want to check with TF guys about the double-hole-check on 32bit https://codereview.chromium.org/1995453002/diff/80001/src/code-stub-assembler.cc File src/code-stub-assembler.cc (right): https://codereview.chromium.org/1995453002/diff/80001/src/code-stub-assembler... src/code-stub-assembler.cc:1397: Variable var_entry(this, MachineType::PointerRepresentation()); Shouldn't this be kWord32 as well? https://codereview.chromium.org/1995453002/diff/80001/src/code-stub-assembler... src/code-stub-assembler.cc:1430: Word32Shl(hash, Int32Constant(15))); What about explicitly sharing these constants with runtime code? https://codereview.chromium.org/1995453002/diff/80001/src/code-stub-assembler... src/code-stub-assembler.cc:1571: NameDictionaryLookup<GlobalDictionary>(dictionary, unique_name, if_found, JS_GLOBAL_OBJECT_TYPE <= LAST_SPECIAL_RECEIVER_TYPE, so I presume this isn't tested. Perhaps we shouldn't add until that point?
Patchset #4 (id:100001) has been deleted
Patchset #4 (id:120001) has been deleted
Patchset #4 (id:140001) has been deleted
Description was changed from ========== Extend HasProperty stub with dictionary-mode and double-elements objects support. This CL also replaces some Branch() usages with GotoIf/GotoUnless. BUG=v8:2743 LOG=Y ========== to ========== Extend HasProperty stub with dictionary-mode, string wrapper and double-elements objects support. This CL also replaces some Branch() usages with GotoIf/GotoUnless. BUG=v8:2743 LOG=Y ==========
The CQ bit was checked by ishell@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/1995453002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1995453002/160001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_android_arm_compile_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_android_arm_compile_rel/...) v8_linux_dbg_ng on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/6466) v8_linux_mips64el_compile_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...) v8_linux_rel_ng on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/6437) v8_presubmit on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/16136)
Patchset #5 (id:180001) has been deleted
The CQ bit was checked by ishell@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/1995453002/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1995453002/200001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux_gcc_compile_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...)
Addressed comments and added unit tests. https://codereview.chromium.org/1995453002/diff/80001/src/code-stub-assembler.cc File src/code-stub-assembler.cc (right): https://codereview.chromium.org/1995453002/diff/80001/src/code-stub-assembler... src/code-stub-assembler.cc:1397: Variable var_entry(this, MachineType::PointerRepresentation()); On 2016/05/19 14:50:22, Toon Verwaest wrote: > Shouldn't this be kWord32 as well? Done. https://codereview.chromium.org/1995453002/diff/80001/src/code-stub-assembler... src/code-stub-assembler.cc:1430: Word32Shl(hash, Int32Constant(15))); On 2016/05/19 14:50:22, Toon Verwaest wrote: > What about explicitly sharing these constants with runtime code? I think we can just rely on a respective unit test results instead. https://codereview.chromium.org/1995453002/diff/80001/src/code-stub-assembler... src/code-stub-assembler.cc:1571: NameDictionaryLookup<GlobalDictionary>(dictionary, unique_name, if_found, On 2016/05/19 14:50:22, Toon Verwaest wrote: > JS_GLOBAL_OBJECT_TYPE <= LAST_SPECIAL_RECEIVER_TYPE, so I presume this isn't > tested. Perhaps we shouldn't add until that point? Fixed and added unit tests. https://codereview.chromium.org/1995453002/diff/200001/test/cctest/test-code-... File test/cctest/test-code-stub-assembler.cc (right): https://codereview.chromium.org/1995453002/diff/200001/test/cctest/test-code-... test/cctest/test-code-stub-assembler.cc:389: New tests are below.
The CQ bit was checked by ishell@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/1995453002/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1995453002/220001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by ishell@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1995453002/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1995453002/220001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: v8_linux_chromium_gn_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_chromium_gn_rel/bu...)
The CQ bit was checked by ishell@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/1995453002/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1995453002/240001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux_gcc_compile_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...)
The CQ bit was checked by ishell@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/1995453002/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1995453002/280001
Patchset #8 (id:260001) has been deleted
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 ishell@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from verwaest@chromium.org Link to the patchset: https://codereview.chromium.org/1995453002/#ps280001 (title: "Fix for GCC compilation issue")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1995453002/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1995453002/280001
Message was sent while issue was closed.
Description was changed from ========== Extend HasProperty stub with dictionary-mode, string wrapper and double-elements objects support. This CL also replaces some Branch() usages with GotoIf/GotoUnless. BUG=v8:2743 LOG=Y ========== to ========== Extend HasProperty stub with dictionary-mode, string wrapper and double-elements objects support. This CL also replaces some Branch() usages with GotoIf/GotoUnless. BUG=v8:2743 LOG=Y ==========
Message was sent while issue was closed.
Committed patchset #8 (id:280001)
Message was sent while issue was closed.
Description was changed from ========== Extend HasProperty stub with dictionary-mode, string wrapper and double-elements objects support. This CL also replaces some Branch() usages with GotoIf/GotoUnless. BUG=v8:2743 LOG=Y ========== to ========== Extend HasProperty stub with dictionary-mode, string wrapper and double-elements objects support. This CL also replaces some Branch() usages with GotoIf/GotoUnless. BUG=v8:2743 LOG=Y Committed: https://crrev.com/24066b6df4259b302edfa1db884c479008776a7e Cr-Commit-Position: refs/heads/master@{#36657} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/24066b6df4259b302edfa1db884c479008776a7e Cr-Commit-Position: refs/heads/master@{#36657}
Message was sent while issue was closed.
A revert of this CL (patchset #8 id:280001) has been created in https://codereview.chromium.org/2028333002/ by ishell@chromium.org. The reason for reverting is: There are crashes on Win32 and Win64 bots..
Message was sent while issue was closed.
Description was changed from ========== Extend HasProperty stub with dictionary-mode, string wrapper and double-elements objects support. This CL also replaces some Branch() usages with GotoIf/GotoUnless. BUG=v8:2743 LOG=Y Committed: https://crrev.com/24066b6df4259b302edfa1db884c479008776a7e Cr-Commit-Position: refs/heads/master@{#36657} ========== to ========== Extend HasProperty stub with dictionary-mode, string wrapper and double-elements objects support. This CL also replaces some Branch() usages with GotoIf/GotoUnless. BUG=v8:2743 LOG=Y Committed: https://crrev.com/24066b6df4259b302edfa1db884c479008776a7e Cr-Commit-Position: refs/heads/master@{#36657} ==========
Description was changed from ========== Extend HasProperty stub with dictionary-mode, string wrapper and double-elements objects support. This CL also replaces some Branch() usages with GotoIf/GotoUnless. BUG=v8:2743 LOG=Y Committed: https://crrev.com/24066b6df4259b302edfa1db884c479008776a7e Cr-Commit-Position: refs/heads/master@{#36657} ========== to ========== [stubs] Extend HasProperty stub with dictionary-mode, string wrapper and double-elements objects support. This CL also replaces some Branch() usages with GotoIf/GotoUnless. BUG=v8:2743 LOG=Y Committed: https://crrev.com/24066b6df4259b302edfa1db884c479008776a7e Cr-Commit-Position: refs/heads/master@{#36657} ==========
Patchset #10 (id:320001) has been deleted
The CQ bit was checked by ishell@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/1995453002/340001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1995453002/340001
Description was changed from ========== [stubs] Extend HasProperty stub with dictionary-mode, string wrapper and double-elements objects support. This CL also replaces some Branch() usages with GotoIf/GotoUnless. BUG=v8:2743 LOG=Y Committed: https://crrev.com/24066b6df4259b302edfa1db884c479008776a7e Cr-Commit-Position: refs/heads/master@{#36657} ========== to ========== [stubs] Extend HasProperty stub with dictionary-mode, string wrapper and double-elements objects support. This CL also replaces some Branch() usages with GotoIf/GotoUnless. (This is a reland after fixing issues that prevented this CL from landing in other CLs). BUG=v8:2743 LOG=Y Committed: https://crrev.com/24066b6df4259b302edfa1db884c479008776a7e Cr-Commit-Position: refs/heads/master@{#36657} ==========
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 ishell@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from verwaest@chromium.org Link to the patchset: https://codereview.chromium.org/1995453002/#ps340001 (title: "Drive-by fix: replace Load()s with LoadObjectField()s")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1995453002/340001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1995453002/340001
Message was sent while issue was closed.
Description was changed from ========== [stubs] Extend HasProperty stub with dictionary-mode, string wrapper and double-elements objects support. This CL also replaces some Branch() usages with GotoIf/GotoUnless. (This is a reland after fixing issues that prevented this CL from landing in other CLs). BUG=v8:2743 LOG=Y Committed: https://crrev.com/24066b6df4259b302edfa1db884c479008776a7e Cr-Commit-Position: refs/heads/master@{#36657} ========== to ========== [stubs] Extend HasProperty stub with dictionary-mode, string wrapper and double-elements objects support. This CL also replaces some Branch() usages with GotoIf/GotoUnless. (This is a reland after fixing issues that prevented this CL from landing in other CLs). BUG=v8:2743 LOG=Y Committed: https://crrev.com/24066b6df4259b302edfa1db884c479008776a7e Cr-Commit-Position: refs/heads/master@{#36657} ==========
Message was sent while issue was closed.
Committed patchset #10 (id:340001)
Message was sent while issue was closed.
Description was changed from ========== [stubs] Extend HasProperty stub with dictionary-mode, string wrapper and double-elements objects support. This CL also replaces some Branch() usages with GotoIf/GotoUnless. (This is a reland after fixing issues that prevented this CL from landing in other CLs). BUG=v8:2743 LOG=Y Committed: https://crrev.com/24066b6df4259b302edfa1db884c479008776a7e Cr-Commit-Position: refs/heads/master@{#36657} ========== to ========== [stubs] Extend HasProperty stub with dictionary-mode, string wrapper and double-elements objects support. This CL also replaces some Branch() usages with GotoIf/GotoUnless. (This is a reland after fixing issues that prevented this CL from landing in other CLs). BUG=v8:2743 LOG=Y Committed: https://crrev.com/24066b6df4259b302edfa1db884c479008776a7e Cr-Commit-Position: refs/heads/master@{#36657} Committed: https://crrev.com/3c4f903e561d23bbec3b33767b51699be79f7b2d Cr-Commit-Position: refs/heads/master@{#36686} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/3c4f903e561d23bbec3b33767b51699be79f7b2d Cr-Commit-Position: refs/heads/master@{#36686}
Message was sent while issue was closed.
On 2016/06/02 15:03:08, commit-bot: I haz the power wrote: > Patchset 10 (id:??) landed as > https://crrev.com/3c4f903e561d23bbec3b33767b51699be79f7b2d > Cr-Commit-Position: refs/heads/master@{#36686} I saw a crash under v8_enable_slow_dchecks, which showed up on the bots once so far: https://build.chromium.org/p/client.v8/builders/V8%20Linux%20-%20noi18n%20-%2... Not sure why it started crashing, though I'm fairly confident it wasn't due to my change (https://chromium.googlesource.com/v8/v8/+/4cc2a73185f2d86ac6b18fd8935bb1f72ad...)
Message was sent while issue was closed.
On 2016/06/04 00:03:36, adamk wrote: > On 2016/06/02 15:03:08, commit-bot: I haz the power wrote: > > Patchset 10 (id:??) landed as > > https://crrev.com/3c4f903e561d23bbec3b33767b51699be79f7b2d > > Cr-Commit-Position: refs/heads/master@{#36686} > > I saw a crash under v8_enable_slow_dchecks, which showed up on the bots once so > far: > > https://build.chromium.org/p/client.v8/builders/V8%20Linux%20-%20noi18n%20-%2... > > Not sure why it started crashing, though I'm fairly confident it wasn't due to > my change > (https://chromium.googlesource.com/v8/v8/+/4cc2a73185f2d86ac6b18fd8935bb1f72ad...) Fix is on the way: https://codereview.chromium.org/2044003004/ |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
