Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(100)

Issue 1747293002: Instantiate instance templates instead of doing construct calls (Closed)

Created:
4 years, 9 months ago by jochen (gone - plz use gerrit)
Modified:
4 years, 9 months ago
Reviewers:
haraken, Toon Verwaest
CC:
chromium-reviews, blink-reviews, blink-reviews-bindings_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Instantiate instance templates instead of doing construct calls This reduces the calls to V8 to create new wrappers. We also don't rely on invoking the function anyways, but instead cache results. BUG= R=verwaest@chromium.org Committed: https://crrev.com/5800d1a48a2f4c2aab7cf875b164b08328c7acf4 Cr-Commit-Position: refs/heads/master@{#378497}

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+1 line, -4 lines) Patch
M third_party/WebKit/Source/bindings/core/v8/V8DOMWrapper.cpp View 1 chunk +1 line, -4 lines 0 comments Download

Messages

Total messages: 10 (2 generated)
jochen (gone - plz use gerrit)
4 years, 9 months ago (2016-03-01 16:10:34 UTC) #1
Toon Verwaest
lgtm
4 years, 9 months ago (2016-03-01 17:20:08 UTC) #2
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1747293002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1747293002/1
4 years, 9 months ago (2016-03-01 17:29:48 UTC) #4
haraken
LGTM
4 years, 9 months ago (2016-03-01 17:33:48 UTC) #5
commit-bot: I haz the power
Committed patchset #1 (id:1)
4 years, 9 months ago (2016-03-01 18:57:10 UTC) #6
commit-bot: I haz the power
Patchset 1 (id:??) landed as https://crrev.com/5800d1a48a2f4c2aab7cf875b164b08328c7acf4 Cr-Commit-Position: refs/heads/master@{#378497}
4 years, 9 months ago (2016-03-01 18:58:31 UTC) #8
Ken Russell (switch to Gerrit)
A revert of this CL (patchset #1 id:1) has been created in https://codereview.chromium.org/1753363002/ by kbr@chromium.org. ...
4 years, 9 months ago (2016-03-02 19:23:47 UTC) #9
jochen (gone - plz use gerrit)
4 years, 9 months ago (2016-03-02 21:27:29 UTC) #10
Message was sent while issue was closed.
On 2016/03/02 at 19:23:47, kbr wrote:
> A revert of this CL (patchset #1 id:1) has been created in
https://codereview.chromium.org/1753363002/ by kbr@chromium.org.
> 
> The reason for reverting is: It looks like this caused the
WebglConformance.conformance_misc_expando_loss test to start failing (only on
Win x64, strangely). This test verifies that expando properties added to
JavaScript instances which are kept alive via hidden properties are preserved
during GC:
> 

Can you help me understand how you came to this conclusion? I'm not sure I
understand the test output, but it looks like it's just timing out, or am I
missing some error message?

I looked at the test source, but it does a bunch of stuff, so I'm not sure where
to start debugging

https://build.chromium.org/p/chromium.gpu.fyi/builders/Win7%20x64%20Debug%20%...
> 
> In the future it would be really helpful if you filed bugs about changes like
this so that lands, reverts and re-lands were all referenced from one place.
> .

This change is just inlining code - and we have a bunch of layout tests that
cover persistence of properties across GC... It didn't seem like this would
warrant a bug.

Powered by Google App Engine
This is Rietveld 408576698