Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2751263003:
Give v8::Eternal a direct reference to the handle. (Closed)
DescriptionGive v8::Eternal a direct reference to the handle.
This makes it more similar to other handle types (like PersistentBase),
by simply storing an i::Object** cast to T*. This means that it is not
necessary to look up the handle in the eternal handles table to access
the underlying value.
Like the built-in roots (null, etc.), an eternal handle can never be
destroyed, so we don't even need to allocate a separate local handle.
Instead, the Local<T> can point directly at the eternal reference.
This makes Eternal<T>::Get trivial.
Review-Url: https://codereview.chromium.org/2751263003
Cr-Commit-Position: refs/heads/master@{#43912}
Committed: https://chromium.googlesource.com/v8/v8/+/4acdb5eec2c79331c47081c23f7d51d3244a2bf0
Patch Set 1 #Patch Set 2 : Even cheaper -- this makes conversion to Local<T> totally free. #Patch Set 3 : add needed reinterpret_cast #Patch Set 4 : . #Patch Set 5 : Merge branch 'master' into slim-eternal #Messages
Total messages: 34 (27 generated)
The CQ bit was checked by jbroman@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/24290) v8_win_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng/builds/24379)
The CQ bit was checked by jbroman@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_avx2_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...) 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_mac_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/19185) v8_win64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel_ng/builds/24291) v8_win_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng/builds/24380)
The CQ bit was checked by jbroman@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_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/22700) v8_linux64_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng_triggered...)
The CQ bit was checked by jbroman@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.
Description was changed from ========== Give v8::Eternal a direct reference to the handle. The i::Object* in the eternal handle blocks never move. So we can directly hold the address of it in the v8::Eternal, like how PersistentBase works. This makes conversion to a v8::Local cheaper, as we only pay the cost of opening a handle, but don't have to indirect through the eternal table. BUG= ========== to ========== Give v8::Eternal a direct reference to the handle. This makes it more similar to other handle types (like PersistentBase), by simply storing an i::Object** cast to T*. This means that it is not necessary to look up the handle in the eternal handles table to access the underlying value. Like the built-in roots (null, etc.), an eternal handle can never be destroyed, so we don't even need to allocate a separate local handle. Instead, the Local<T> can point directly at the eternal reference. This makes Eternal<T>::Get completely free. ==========
Description was changed from ========== Give v8::Eternal a direct reference to the handle. This makes it more similar to other handle types (like PersistentBase), by simply storing an i::Object** cast to T*. This means that it is not necessary to look up the handle in the eternal handles table to access the underlying value. Like the built-in roots (null, etc.), an eternal handle can never be destroyed, so we don't even need to allocate a separate local handle. Instead, the Local<T> can point directly at the eternal reference. This makes Eternal<T>::Get completely free. ========== to ========== Give v8::Eternal a direct reference to the handle. This makes it more similar to other handle types (like PersistentBase), by simply storing an i::Object** cast to T*. This means that it is not necessary to look up the handle in the eternal handles table to access the underlying value. Like the built-in roots (null, etc.), an eternal handle can never be destroyed, so we don't even need to allocate a separate local handle. Instead, the Local<T> can point directly at the eternal reference. This makes Eternal<T>::Get free. ==========
Description was changed from ========== Give v8::Eternal a direct reference to the handle. This makes it more similar to other handle types (like PersistentBase), by simply storing an i::Object** cast to T*. This means that it is not necessary to look up the handle in the eternal handles table to access the underlying value. Like the built-in roots (null, etc.), an eternal handle can never be destroyed, so we don't even need to allocate a separate local handle. Instead, the Local<T> can point directly at the eternal reference. This makes Eternal<T>::Get free. ========== to ========== Give v8::Eternal a direct reference to the handle. This makes it more similar to other handle types (like PersistentBase), by simply storing an i::Object** cast to T*. This means that it is not necessary to look up the handle in the eternal handles table to access the underlying value. Like the built-in roots (null, etc.), an eternal handle can never be destroyed, so we don't even need to allocate a separate local handle. Instead, the Local<T> can point directly at the eternal reference. This makes Eternal<T>::Get trivial. ==========
jbroman@chromium.org changed reviewers: + jochen@chromium.org
V8::GetEternal showed up as a little over 1% when I was profiling an approach to use eternal handles to hold IDL dictionary keys (skipping the internalization process) on the DOMMatrixReadOnly multiplication benchmark. Not huge, but more significant than I'd expected. This seems like it both makes v8::Eternal more similar to other handle types, and makes the compiled code both faster and smaller. In my case, it causes things like "get the 9th v8::Eternal in this array as a v8::Local (so that it can be passed on a API function)" to compile into a single x86 "mov 0x40(%rax),%rdx".
jochen@chromium.org changed reviewers: + ulan@chromium.org
uh, hum, not sure this is correct Ulan is probably a better reviewer for this
lgtm, much cleaner this way. I don't know the reason why indexes were used for accessing eternals. I checked that the backing store for the handles does not move, so using addresses directly should be fine.
The CQ bit was checked by jbroman@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ulan@chromium.org Link to the patchset: https://codereview.chromium.org/2751263003/#ps80001 (title: "Merge branch 'master' into slim-eternal")
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: v8_linux_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/22735) v8_linux_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng_triggered/b...)
The CQ bit was checked by jbroman@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": 80001, "attempt_start_ts": 1489770292574370,
"parent_rev": "7bd0c1d5bb899d67b8a785ee4821e5a30c3bc24b", "commit_rev":
"4acdb5eec2c79331c47081c23f7d51d3244a2bf0"}
Message was sent while issue was closed.
Description was changed from ========== Give v8::Eternal a direct reference to the handle. This makes it more similar to other handle types (like PersistentBase), by simply storing an i::Object** cast to T*. This means that it is not necessary to look up the handle in the eternal handles table to access the underlying value. Like the built-in roots (null, etc.), an eternal handle can never be destroyed, so we don't even need to allocate a separate local handle. Instead, the Local<T> can point directly at the eternal reference. This makes Eternal<T>::Get trivial. ========== to ========== Give v8::Eternal a direct reference to the handle. This makes it more similar to other handle types (like PersistentBase), by simply storing an i::Object** cast to T*. This means that it is not necessary to look up the handle in the eternal handles table to access the underlying value. Like the built-in roots (null, etc.), an eternal handle can never be destroyed, so we don't even need to allocate a separate local handle. Instead, the Local<T> can point directly at the eternal reference. This makes Eternal<T>::Get trivial. Review-Url: https://codereview.chromium.org/2751263003 Cr-Commit-Position: refs/heads/master@{#43912} Committed: https://chromium.googlesource.com/v8/v8/+/4acdb5eec2c79331c47081c23f7d51d3244... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/v8/v8/+/4acdb5eec2c79331c47081c23f7d51d3244... |
