Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2781303002:
[ES6 modules] Introduce ScriptModuleHash (Closed)
|
Created:
3 years, 8 months ago by kouhei (in TOK) Modified:
3 years, 8 months ago CC:
chromium-reviews, blink-reviews, blink-reviews-bindings_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[ES6 modules] Introduce ScriptModuleHash
This CL introduces ScriptModuleHash implementation, which allows ScriptModule to be used as a wtf::Hash{Table,Set,Map} key/value.
BUG=594639
Review-Url: https://codereview.chromium.org/2781303002
Cr-Commit-Position: refs/heads/master@{#461979}
Committed: https://chromium.googlesource.com/chromium/src/+/41f44b42ba111f77d99f545f3a64022d28577638
Patch Set 1 #Patch Set 2 : remove defaults #
Total comments: 8
Patch Set 3 : add_tests #
Total comments: 4
Patch Set 4 : noempty #
Total comments: 5
Patch Set 5 : rebased #Patch Set 6 : rebased #
Dependent Patchsets: Messages
Total messages: 37 (15 generated)
kouhei@chromium.org changed reviewers: + adamk@chromium.org, haraken@chromium.org, neis@chromium.org, yhirano@chromium.org
Description was changed from
==========
[ES6 module] Introduce ScriptModuleHash
This CL introduces ScriptModuleHash implementation, which allows ScriptModule to
be used as a wtf::Hash{Table,Set,Map} key/value.
BUG=594639
==========
to
==========
[ES6 modules] Introduce ScriptModuleHash
This CL introduces ScriptModuleHash implementation, which allows ScriptModule to
be used as a wtf::Hash{Table,Set,Map} key/value.
BUG=594639
==========
https://codereview.chromium.org/2781303002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/ScriptModule.h (right): https://codereview.chromium.org/2781303002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/ScriptModule.h:39: if (m_module.isHashTableDeletedValue() && isHashTableDeletedValue() && rhs.isHashTableDeletedValue() is easier to understand. https://codereview.chromium.org/2781303002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/ScriptModule.h:42: if (isHashTableDeltedValue() || rhs.isHashTableDeletedValue()) return false; https://codereview.chromium.org/2781303002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/ScriptModule.h:68: return static_cast<unsigned>(key.m_identityHash); This cast is not needed.
I'd like to have a test. https://codereview.chromium.org/2781303002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/ScriptModule.h (right): https://codereview.chromium.org/2781303002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/ScriptModule.h:45: if (lhsp == rhsp) Nit: Blink prefers a fully qualified name.
https://codereview.chromium.org/2781303002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/ScriptModule.h (right): https://codereview.chromium.org/2781303002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/ScriptModule.h:39: if (m_module.isHashTableDeletedValue() && On 2017/03/30 07:35:09, yhirano (slow) wrote: > isHashTableDeletedValue() && rhs.isHashTableDeletedValue() is easier to > understand. Done. https://codereview.chromium.org/2781303002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/ScriptModule.h:42: On 2017/03/30 07:35:09, yhirano (slow) wrote: > if (isHashTableDeltedValue() || rhs.isHashTableDeletedValue()) > return false; Done. https://codereview.chromium.org/2781303002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/ScriptModule.h:45: if (lhsp == rhsp) On 2017/03/30 13:20:14, haraken wrote: > > Nit: Blink prefers a fully qualified name. Done. https://codereview.chromium.org/2781303002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/ScriptModule.h:68: return static_cast<unsigned>(key.m_identityHash); On 2017/03/30 07:35:09, yhirano (slow) wrote: > This cast is not needed. Done.
lgtm
haraken@chromium.org changed reviewers: + yutak@chromium.org
+yutak for HashTraits. https://codereview.chromium.org/2781303002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/ScriptModule.cpp (right): https://codereview.chromium.org/2781303002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/ScriptModule.cpp:13: if (!module.IsEmpty()) Is it possible that m_module is empty? Then compile, instantiate should fail... https://codereview.chromium.org/2781303002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/ScriptModule.h (right): https://codereview.chromium.org/2781303002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/ScriptModule.h:74: return key.m_identityHash; Why can't we simply use key.m_module.get() for the hash value?
https://codereview.chromium.org/2781303002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/ScriptModule.cpp (right): https://codereview.chromium.org/2781303002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/ScriptModule.cpp:13: if (!module.IsEmpty()) On 2017/04/04 05:26:06, haraken wrote: > > Is it possible that m_module is empty? Then compile, instantiate should fail... Addressed in https://codereview.chromium.org/2798533002/ https://codereview.chromium.org/2781303002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/ScriptModule.h (right): https://codereview.chromium.org/2781303002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/ScriptModule.h:74: return key.m_identityHash; On 2017/04/04 05:26:06, haraken wrote: > > Why can't we simply use key.m_module.get() for the hash value? Discussed offline. No, because ScriptModule::resolveModuleCallback to be introduced in the below CL may create ScriptModule of same underlying v8::Module w/ a different v8::ScopedPersistent instance: https://codereview.chromium.org/2785983002/diff/60001/third_party/WebKit/Sour...
On 2017/04/04 05:54:32, kouhei wrote: > https://codereview.chromium.org/2781303002/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/bindings/core/v8/ScriptModule.cpp (right): > > https://codereview.chromium.org/2781303002/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/bindings/core/v8/ScriptModule.cpp:13: if > (!module.IsEmpty()) > On 2017/04/04 05:26:06, haraken wrote: > > > > Is it possible that m_module is empty? Then compile, instantiate should > fail... > > Addressed in https://codereview.chromium.org/2798533002/ > > https://codereview.chromium.org/2781303002/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/bindings/core/v8/ScriptModule.h (right): > > https://codereview.chromium.org/2781303002/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/bindings/core/v8/ScriptModule.h:74: return > key.m_identityHash; > On 2017/04/04 05:26:06, haraken wrote: > > > > Why can't we simply use key.m_module.get() for the hash value? > > Discussed offline. > No, because ScriptModule::resolveModuleCallback to be introduced in the below CL > may create ScriptModule of same underlying v8::Module w/ a different > v8::ScopedPersistent instance: > https://codereview.chromium.org/2785983002/diff/60001/third_party/WebKit/Sour... LGTM if yutak@ is happy with the HashTraits.
LGTM regarding hash function and traits. Using V8's hash value looks uncommon, but in this case that should be fine. https://codereview.chromium.org/2781303002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/ScriptModule.h (right): https://codereview.chromium.org/2781303002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/ScriptModule.h:64: unsigned m_identityHash = 0; optional: If you really care about the space waste, you might want to just call GetIdentityHash() every time it is requested. That should be okay if we can assume GetIdentityHash() is fast. https://codereview.chromium.org/2781303002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/ScriptModule.h:74: return key.m_identityHash; Hm. This means you use a hash value returned from V8 for WTF::HashTable, right? I've never seen this pattern before, but I can find no reasons why this won't work, so this should be okay...
https://codereview.chromium.org/2781303002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/ScriptModule.h (right): https://codereview.chromium.org/2781303002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/ScriptModule.h:64: unsigned m_identityHash = 0; On 2017/04/04 06:21:51, Yuta Kitamura wrote: > optional: If you really care about the space waste, you might > want to just call GetIdentityHash() every time it is requested. > That should be okay if we can assume GetIdentityHash() is fast. I want to have hash work w/o ScriptState*. :/ GetIdentityHash requires v8::Isolate*
The CQ bit was checked by kouhei@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yhirano@chromium.org Link to the patchset: https://codereview.chromium.org/2781303002/#ps60001 (title: "noempty")
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
Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by kouhei@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org, yutak@chromium.org, yhirano@chromium.org Link to the patchset: https://codereview.chromium.org/2781303002/#ps80001 (title: "rebased")
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
Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
https://codereview.chromium.org/2781303002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/ScriptModule.h (right): https://codereview.chromium.org/2781303002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/ScriptModule.h:64: unsigned m_identityHash = 0; On 2017/04/04 06:36:55, kouhei wrote: > On 2017/04/04 06:21:51, Yuta Kitamura wrote: > > optional: If you really care about the space waste, you might > > want to just call GetIdentityHash() every time it is requested. > > That should be okay if we can assume GetIdentityHash() is fast. > > I want to have hash work w/o ScriptState*. :/ > GetIdentityHash requires v8::Isolate* I don't understand this comment: v8::Module::GetIdentityHash() takes no arguments. Is the problem that you'd need to create a v8::Local in order to call the underlying GetIdentityHash() method, and that requires a v8::Isolate?
On 2017/04/05 01:01:03, adamk wrote: > https://codereview.chromium.org/2781303002/diff/60001/third_party/WebKit/Sour... > File third_party/WebKit/Source/bindings/core/v8/ScriptModule.h (right): > > https://codereview.chromium.org/2781303002/diff/60001/third_party/WebKit/Sour... > third_party/WebKit/Source/bindings/core/v8/ScriptModule.h:64: unsigned > m_identityHash = 0; > On 2017/04/04 06:36:55, kouhei wrote: > > On 2017/04/04 06:21:51, Yuta Kitamura wrote: > > > optional: If you really care about the space waste, you might > > > want to just call GetIdentityHash() every time it is requested. > > > That should be okay if we can assume GetIdentityHash() is fast. > > > > I want to have hash work w/o ScriptState*. :/ > > GetIdentityHash requires v8::Isolate* > > I don't understand this comment: v8::Module::GetIdentityHash() takes no > arguments. Is the problem that you'd need to create a v8::Local in order to call > the underlying GetIdentityHash() method, and that requires a v8::Isolate? Yes
https://codereview.chromium.org/2781303002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/ScriptModule.h (right): https://codereview.chromium.org/2781303002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/ScriptModule.h:64: unsigned m_identityHash = 0; On 2017/04/05 01:01:03, adamk wrote: > On 2017/04/04 06:36:55, kouhei wrote: > > On 2017/04/04 06:21:51, Yuta Kitamura wrote: > > > optional: If you really care about the space waste, you might > > > want to just call GetIdentityHash() every time it is requested. > > > That should be okay if we can assume GetIdentityHash() is fast. > > > > I want to have hash work w/o ScriptState*. :/ > > GetIdentityHash requires v8::Isolate* > > I don't understand this comment: v8::Module::GetIdentityHash() takes no > arguments. Is the problem that you'd need to create a v8::Local in order to call > the underlying GetIdentityHash() method, and that requires a v8::Isolate? Ok, makes sense. It's too bad the API is over-restrictive in this way (it'd be completely safe to access the hash through the persistent handle), but I guess that's something we have to pay for now that the Persistent API is safer.
The CQ bit was checked by kouhei@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org, yutak@chromium.org, yhirano@chromium.org Link to the patchset: https://codereview.chromium.org/2781303002/#ps100001 (title: "rebased")
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
Try jobs failed on following builders: ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by kouhei@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1491362046888100,
"parent_rev": "43c5f09a24e0f27ffa1009917405123e2844d747", "commit_rev":
"41f44b42ba111f77d99f545f3a64022d28577638"}
Message was sent while issue was closed.
Description was changed from
==========
[ES6 modules] Introduce ScriptModuleHash
This CL introduces ScriptModuleHash implementation, which allows ScriptModule to
be used as a wtf::Hash{Table,Set,Map} key/value.
BUG=594639
==========
to
==========
[ES6 modules] Introduce ScriptModuleHash
This CL introduces ScriptModuleHash implementation, which allows ScriptModule to
be used as a wtf::Hash{Table,Set,Map} key/value.
BUG=594639
Review-Url: https://codereview.chromium.org/2781303002
Cr-Commit-Position: refs/heads/master@{#461979}
Committed:
https://chromium.googlesource.com/chromium/src/+/41f44b42ba111f77d99f545f3a64...
==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/41f44b42ba111f77d99f545f3a64... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
