New issue
Advanced search Search tips

Issue 611864 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: May 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Bug

Blocked on:
issue 608576

Blocking:
issue 581997
issue 616618



Sign in to add a comment

V8HiddenValue::getHiddenValue is slow

Project Member Reported by kbr@chromium.org, May 13 2016

Issue description

In the fix for  Issue 608576 , several persistent caches of hidden values were added to avoid the need to call V8HiddenValue::getHiddenValue. Calling this lost much of the performance gain of the optimization, as described in the fix for that bug.

The cost can be demonstrated by replacing the body of WebGLRenderingContextBase::preserveObjectWrapper with the following code:


    v8::Isolate* isolate = scriptState->isolate();
    v8::Local<v8::Object> sourceWrapper = sourceObject->newLocalWrapper(isolate);
    v8::Local<v8::Value> cache = V8HiddenValue::getHiddenValue(scriptState, sourceWrapper, hiddenValueName);
    if (cache.IsEmpty()) {
        cache = v8::Array::New(isolate);
        V8HiddenValue::setHiddenValue(scriptState, sourceWrapper, hiddenValueName, cache);
    }
    if (targetObject) {
        v8::Maybe<bool> result = cache->ToObject(scriptState->context()).ToLocalChecked()->Set(
            scriptState->context(), index, targetObject->newLocalWrapper(isolate));
        ASSERT_UNUSED(result, result.FromMaybe(true));
    } else {
        v8::Maybe<bool> result = cache->ToObject(scriptState->context()).ToLocalChecked()->Set(
            scriptState->context(), index, v8::Null(isolate));
        ASSERT_UNUSED(result, result.FromMaybe(true));
    }


and re-running the benchmark:

./tools/perf/run_benchmark --browser=release smoothness.tough_webgl_cases

To run just the desired benchmark, comment out everything but the Animometer test case from ToughWebglCasesPageSet in src/tools/perf/page_sets/tough_webgl_cases.py .

Speeding this up would improve several areas of the DOM bindings.

 

Comment 1 by bashi@chromium.org, May 15 2016

Cc: peria@chromium.org
Owner: yukishiino@chromium.org
Status: Started (was: Untriaged)

Comment 3 by kbr@chromium.org, May 28 2016

Blocking: 581997
Project Member

Comment 4 by bugdroid1@chromium.org, May 30 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4419cbd172b5d77e0e33b7622666e523d8b20afa

commit 4419cbd172b5d77e0e33b7622666e523d8b20afa
Author: yukishiino <yukishiino@chromium.org>
Date: Mon May 30 08:27:29 2016

binding: Reimplements V8HiddenValue as V8PrivateProperty.

V8HiddenValue is very slow.  This CL implements it again as
V8PrivateProperty so that it will be efficient.  Also makes it clear
which interface and which attribute, etc. is using which private symbol.
Note that "Class#property" is recommended as symbol names by V8.

Following CLs will replace V8HiddenValue with V8PrivateProperty gradually.

BUG= 611864 

Review-Url: https://codereview.chromium.org/2007463003
Cr-Commit-Position: refs/heads/master@{#396694}

[modify] https://crrev.com/4419cbd172b5d77e0e33b7622666e523d8b20afa/third_party/WebKit/Source/bindings/core/v8/V8HiddenValue.h
[modify] https://crrev.com/4419cbd172b5d77e0e33b7622666e523d8b20afa/third_party/WebKit/Source/bindings/core/v8/V8PerIsolateData.cpp
[modify] https://crrev.com/4419cbd172b5d77e0e33b7622666e523d8b20afa/third_party/WebKit/Source/bindings/core/v8/V8PerIsolateData.h
[add] https://crrev.com/4419cbd172b5d77e0e33b7622666e523d8b20afa/third_party/WebKit/Source/bindings/core/v8/V8PrivateProperty.cpp
[add] https://crrev.com/4419cbd172b5d77e0e33b7622666e523d8b20afa/third_party/WebKit/Source/bindings/core/v8/V8PrivateProperty.h
[modify] https://crrev.com/4419cbd172b5d77e0e33b7622666e523d8b20afa/third_party/WebKit/Source/bindings/core/v8/custom/V8MessageEventCustom.cpp
[modify] https://crrev.com/4419cbd172b5d77e0e33b7622666e523d8b20afa/third_party/WebKit/Source/bindings/core/v8/v8.gypi
[modify] https://crrev.com/4419cbd172b5d77e0e33b7622666e523d8b20afa/third_party/WebKit/Source/bindings/modules/v8/V8ServiceWorkerMessageEventInternal.h
[modify] https://crrev.com/4419cbd172b5d77e0e33b7622666e523d8b20afa/third_party/WebKit/Source/core/events/MessageEvent.cpp

Status: Fixed (was: Started)

Comment 6 by kbr@chromium.org, Jun 1 2016

Blocking: 616618
Project Member

Comment 8 by bugdroid1@chromium.org, Mar 31 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/55fd47019c03eac50969cd75ebac4119d3474bd4

commit 55fd47019c03eac50969cd75ebac4119d3474bd4
Author: peria <peria@chromium.org>
Date: Fri Mar 31 05:44:38 2017

Simplify V8PrivateProperty::Symbol methods

Before this CL, those methods took a context as an argument.
But in case of setting/getting private property, the context to
be used is the current realm and it can be taken from the isolate.
Plus, a context in the argument can be wrong if a developer
misunderstands what should be passed.

Hence this CL removes the argument |context| from Symbol's methods
and does a clean-up changes around it.

BUG= 611864 

Review-Url: https://codereview.chromium.org/2785943002
Cr-Commit-Position: refs/heads/master@{#461051}

[modify] https://crrev.com/55fd47019c03eac50969cd75ebac4119d3474bd4/third_party/WebKit/Source/bindings/core/v8/LocalWindowProxy.cpp
[modify] https://crrev.com/55fd47019c03eac50969cd75ebac4119d3474bd4/third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp
[modify] https://crrev.com/55fd47019c03eac50969cd75ebac4119d3474bd4/third_party/WebKit/Source/bindings/core/v8/V0CustomElementConstructorBuilder.cpp
[modify] https://crrev.com/55fd47019c03eac50969cd75ebac4119d3474bd4/third_party/WebKit/Source/bindings/core/v8/V8ErrorHandler.cpp
[modify] https://crrev.com/55fd47019c03eac50969cd75ebac4119d3474bd4/third_party/WebKit/Source/bindings/core/v8/V8Initializer.cpp
[modify] https://crrev.com/55fd47019c03eac50969cd75ebac4119d3474bd4/third_party/WebKit/Source/bindings/core/v8/V8IntersectionObserverCallback.cpp
[modify] https://crrev.com/55fd47019c03eac50969cd75ebac4119d3474bd4/third_party/WebKit/Source/bindings/core/v8/V8MutationCallback.cpp
[modify] https://crrev.com/55fd47019c03eac50969cd75ebac4119d3474bd4/third_party/WebKit/Source/bindings/core/v8/V8NodeFilterCondition.cpp
[modify] https://crrev.com/55fd47019c03eac50969cd75ebac4119d3474bd4/third_party/WebKit/Source/bindings/core/v8/V8PrivateProperty.cpp
[modify] https://crrev.com/55fd47019c03eac50969cd75ebac4119d3474bd4/third_party/WebKit/Source/bindings/core/v8/V8PrivateProperty.h
[modify] https://crrev.com/55fd47019c03eac50969cd75ebac4119d3474bd4/third_party/WebKit/Source/bindings/core/v8/V8ThrowException.cpp
[modify] https://crrev.com/55fd47019c03eac50969cd75ebac4119d3474bd4/third_party/WebKit/Source/bindings/core/v8/custom/V8CustomEventCustom.cpp
[modify] https://crrev.com/55fd47019c03eac50969cd75ebac4119d3474bd4/third_party/WebKit/Source/bindings/core/v8/custom/V8ErrorEventCustom.cpp
[modify] https://crrev.com/55fd47019c03eac50969cd75ebac4119d3474bd4/third_party/WebKit/Source/bindings/core/v8/custom/V8MessageEventCustom.cpp
[modify] https://crrev.com/55fd47019c03eac50969cd75ebac4119d3474bd4/third_party/WebKit/Source/bindings/core/v8/custom/V8PerformanceObserverCustom.cpp
[modify] https://crrev.com/55fd47019c03eac50969cd75ebac4119d3474bd4/third_party/WebKit/Source/bindings/modules/v8/V8ServiceWorkerMessageEventInternal.h
[modify] https://crrev.com/55fd47019c03eac50969cd75ebac4119d3474bd4/third_party/WebKit/Source/bindings/modules/v8/custom/V8IDBObserverCustom.cpp
[modify] https://crrev.com/55fd47019c03eac50969cd75ebac4119d3474bd4/third_party/WebKit/Source/bindings/templates/attributes.cpp.tmpl
[modify] https://crrev.com/55fd47019c03eac50969cd75ebac4119d3474bd4/third_party/WebKit/Source/bindings/templates/interface.cpp.tmpl
[modify] https://crrev.com/55fd47019c03eac50969cd75ebac4119d3474bd4/third_party/WebKit/Source/bindings/tests/results/core/V8TestInterfaceConstructor.cpp
[modify] https://crrev.com/55fd47019c03eac50969cd75ebac4119d3474bd4/third_party/WebKit/Source/bindings/tests/results/core/V8TestInterfaceEventTarget.cpp
[modify] https://crrev.com/55fd47019c03eac50969cd75ebac4119d3474bd4/third_party/WebKit/Source/bindings/tests/results/core/V8TestInterfaceNamedConstructor.cpp
[modify] https://crrev.com/55fd47019c03eac50969cd75ebac4119d3474bd4/third_party/WebKit/Source/bindings/tests/results/core/V8TestInterfaceNamedConstructor2.cpp
[modify] https://crrev.com/55fd47019c03eac50969cd75ebac4119d3474bd4/third_party/WebKit/Source/bindings/tests/results/core/V8TestObject.cpp
[modify] https://crrev.com/55fd47019c03eac50969cd75ebac4119d3474bd4/third_party/WebKit/Source/core/events/MessageEvent.cpp

Project Member

Comment 9 by bugdroid1@chromium.org, Apr 3 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/330bfd236aa4574a1ee370d4b369e755c6fe034d

commit 330bfd236aa4574a1ee370d4b369e755c6fe034d
Author: peria <peria@chromium.org>
Date: Mon Apr 03 09:03:52 2017

Replace V8HiddenValue methods in generated code with V8PrivateProperty methods.

This CL replaces V8HiddenValue with V8PrivateProperty, but it keeps to use dynamic
strings as property keys.  (It is possible to list all currently used properties and cache
them in V8PrivateProperty, but it is not practical for future updates.)

As far as I investigate, a property key "state" was the only key that was used in both
generated code and hand written code (V8PopStateEventCustom.cpp).
So this CL also updates the .cpp file to share the property symbol.

BUG= 611864 

Review-Url: https://codereview.chromium.org/2794453002
Cr-Commit-Position: refs/heads/master@{#461388}

[modify] https://crrev.com/330bfd236aa4574a1ee370d4b369e755c6fe034d/third_party/WebKit/Source/bindings/core/v8/V8HiddenValue.h
[modify] https://crrev.com/330bfd236aa4574a1ee370d4b369e755c6fe034d/third_party/WebKit/Source/bindings/core/v8/V8PrivateProperty.h
[modify] https://crrev.com/330bfd236aa4574a1ee370d4b369e755c6fe034d/third_party/WebKit/Source/bindings/core/v8/custom/V8PopStateEventCustom.cpp
[modify] https://crrev.com/330bfd236aa4574a1ee370d4b369e755c6fe034d/third_party/WebKit/Source/bindings/scripts/v8_attributes.py
[modify] https://crrev.com/330bfd236aa4574a1ee370d4b369e755c6fe034d/third_party/WebKit/Source/bindings/templates/attributes.cpp.tmpl
[modify] https://crrev.com/330bfd236aa4574a1ee370d4b369e755c6fe034d/third_party/WebKit/Source/bindings/templates/interface.cpp.tmpl
[modify] https://crrev.com/330bfd236aa4574a1ee370d4b369e755c6fe034d/third_party/WebKit/Source/bindings/templates/methods.cpp.tmpl
[modify] https://crrev.com/330bfd236aa4574a1ee370d4b369e755c6fe034d/third_party/WebKit/Source/bindings/tests/results/core/V8ArrayBufferView.cpp
[modify] https://crrev.com/330bfd236aa4574a1ee370d4b369e755c6fe034d/third_party/WebKit/Source/bindings/tests/results/core/V8SVGTestInterface.cpp
[modify] https://crrev.com/330bfd236aa4574a1ee370d4b369e755c6fe034d/third_party/WebKit/Source/bindings/tests/results/core/V8TestCallbackFunctions.cpp
[modify] https://crrev.com/330bfd236aa4574a1ee370d4b369e755c6fe034d/third_party/WebKit/Source/bindings/tests/results/core/V8TestIntegerIndexed.cpp
[modify] https://crrev.com/330bfd236aa4574a1ee370d4b369e755c6fe034d/third_party/WebKit/Source/bindings/tests/results/core/V8TestIntegerIndexedGlobal.cpp
[modify] https://crrev.com/330bfd236aa4574a1ee370d4b369e755c6fe034d/third_party/WebKit/Source/bindings/tests/results/core/V8TestIntegerIndexedPrimaryGlobal.cpp
[modify] https://crrev.com/330bfd236aa4574a1ee370d4b369e755c6fe034d/third_party/WebKit/Source/bindings/tests/results/core/V8TestInterface.cpp
[modify] https://crrev.com/330bfd236aa4574a1ee370d4b369e755c6fe034d/third_party/WebKit/Source/bindings/tests/results/core/V8TestInterfaceCheckSecurity.cpp
[modify] https://crrev.com/330bfd236aa4574a1ee370d4b369e755c6fe034d/third_party/WebKit/Source/bindings/tests/results/core/V8TestInterfaceDocument.cpp
[modify] https://crrev.com/330bfd236aa4574a1ee370d4b369e755c6fe034d/third_party/WebKit/Source/bindings/tests/results/core/V8TestInterfaceGarbageCollected.cpp
[modify] https://crrev.com/330bfd236aa4574a1ee370d4b369e755c6fe034d/third_party/WebKit/Source/bindings/tests/results/core/V8TestInterfaceNode.cpp
[modify] https://crrev.com/330bfd236aa4574a1ee370d4b369e755c6fe034d/third_party/WebKit/Source/bindings/tests/results/core/V8TestInterfaceOriginTrialEnabled.cpp
[modify] https://crrev.com/330bfd236aa4574a1ee370d4b369e755c6fe034d/third_party/WebKit/Source/bindings/tests/results/core/V8TestInterfaceSecureContext.cpp
[modify] https://crrev.com/330bfd236aa4574a1ee370d4b369e755c6fe034d/third_party/WebKit/Source/bindings/tests/results/core/V8TestNode.cpp
[modify] https://crrev.com/330bfd236aa4574a1ee370d4b369e755c6fe034d/third_party/WebKit/Source/bindings/tests/results/core/V8TestObject.cpp
[modify] https://crrev.com/330bfd236aa4574a1ee370d4b369e755c6fe034d/third_party/WebKit/Source/bindings/tests/results/core/V8TestTypedefs.cpp
[modify] https://crrev.com/330bfd236aa4574a1ee370d4b369e755c6fe034d/third_party/WebKit/Source/bindings/tests/results/modules/V8TestInterface5.cpp
[modify] https://crrev.com/330bfd236aa4574a1ee370d4b369e755c6fe034d/third_party/WebKit/Source/bindings/tests/results/modules/V8TestInterfacePartial.cpp

Project Member

Comment 11 by bugdroid1@chromium.org, Apr 5 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2acaf7f8acf2931db3d91fa51a2dd0a0e2d0481f

commit 2acaf7f8acf2931db3d91fa51a2dd0a0e2d0481f
Author: peria <peria@chromium.org>
Date: Wed Apr 05 14:59:02 2017

Moves following symbols from V8HiddenValue to V8PrivateProperty
- customElementAttachedCallback
- customElementAttributeChangedCallback
- customElementCreatedCallback
- customElementDetachedCallback
- customElementsRegistryMap
- idbCursorRequest
- internalBodyBuffer
- internalBodyStream
- requestInFetchEvent
- toStringString

and removes following symbols, which are not used.
- testInterfaces
- thenableHiddenPromise
- injectedScriptNative

BUG= 611864 

Review-Url: https://codereview.chromium.org/2777943005
Cr-Commit-Position: refs/heads/master@{#462071}

[modify] https://crrev.com/2acaf7f8acf2931db3d91fa51a2dd0a0e2d0481f/third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp
[modify] https://crrev.com/2acaf7f8acf2931db3d91fa51a2dd0a0e2d0481f/third_party/WebKit/Source/bindings/core/v8/V8HiddenValue.h
[modify] https://crrev.com/2acaf7f8acf2931db3d91fa51a2dd0a0e2d0481f/third_party/WebKit/Source/bindings/core/v8/V8LazyEventListener.cpp
[modify] https://crrev.com/2acaf7f8acf2931db3d91fa51a2dd0a0e2d0481f/third_party/WebKit/Source/bindings/core/v8/V8PrivateProperty.h
[modify] https://crrev.com/2acaf7f8acf2931db3d91fa51a2dd0a0e2d0481f/third_party/WebKit/Source/bindings/core/v8/V8V0CustomElementLifecycleCallbacks.cpp
[modify] https://crrev.com/2acaf7f8acf2931db3d91fa51a2dd0a0e2d0481f/third_party/WebKit/Source/modules/fetch/BodyStreamBuffer.cpp
[modify] https://crrev.com/2acaf7f8acf2931db3d91fa51a2dd0a0e2d0481f/third_party/WebKit/Source/modules/fetch/Request.cpp
[modify] https://crrev.com/2acaf7f8acf2931db3d91fa51a2dd0a0e2d0481f/third_party/WebKit/Source/modules/fetch/Response.cpp
[modify] https://crrev.com/2acaf7f8acf2931db3d91fa51a2dd0a0e2d0481f/third_party/WebKit/Source/modules/indexeddb/IDBCursor.cpp
[modify] https://crrev.com/2acaf7f8acf2931db3d91fa51a2dd0a0e2d0481f/third_party/WebKit/Source/modules/serviceworkers/FetchEvent.cpp
[modify] https://crrev.com/2acaf7f8acf2931db3d91fa51a2dd0a0e2d0481f/third_party/WebKit/Source/modules/serviceworkers/ForeignFetchEvent.cpp

Project Member

Comment 12 by bugdroid1@chromium.org, Apr 7 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5daaa56488f8ae678e47661e861e8e0c6db8e2a8

commit 5daaa56488f8ae678e47661e861e8e0c6db8e2a8
Author: peria <peria@chromium.org>
Date: Fri Apr 07 11:38:29 2017

Move ScriptPromiseProperties from V8HiddenValue to V8PrivateProperty

This change makes V8HiddenValue class stateless, and we can remove
m_hiddenValue in V8PerIsolate.

BUG= 611864 

Review-Url: https://codereview.chromium.org/2774233007
Cr-Commit-Position: refs/heads/master@{#462837}

[modify] https://crrev.com/5daaa56488f8ae678e47661e861e8e0c6db8e2a8/third_party/WebKit/Source/bindings/core/v8/ScriptPromiseProperties.h
[modify] https://crrev.com/5daaa56488f8ae678e47661e861e8e0c6db8e2a8/third_party/WebKit/Source/bindings/core/v8/ScriptPromisePropertyBase.cpp
[modify] https://crrev.com/5daaa56488f8ae678e47661e861e8e0c6db8e2a8/third_party/WebKit/Source/bindings/core/v8/ScriptPromisePropertyBase.h
[modify] https://crrev.com/5daaa56488f8ae678e47661e861e8e0c6db8e2a8/third_party/WebKit/Source/bindings/core/v8/ScriptPromisePropertyTest.cpp
[modify] https://crrev.com/5daaa56488f8ae678e47661e861e8e0c6db8e2a8/third_party/WebKit/Source/bindings/core/v8/V8EventListenerHelper.h
[modify] https://crrev.com/5daaa56488f8ae678e47661e861e8e0c6db8e2a8/third_party/WebKit/Source/bindings/core/v8/V8HiddenValue.cpp
[modify] https://crrev.com/5daaa56488f8ae678e47661e861e8e0c6db8e2a8/third_party/WebKit/Source/bindings/core/v8/V8HiddenValue.h
[modify] https://crrev.com/5daaa56488f8ae678e47661e861e8e0c6db8e2a8/third_party/WebKit/Source/bindings/core/v8/V8PerIsolateData.cpp
[modify] https://crrev.com/5daaa56488f8ae678e47661e861e8e0c6db8e2a8/third_party/WebKit/Source/bindings/core/v8/V8PerIsolateData.h
[modify] https://crrev.com/5daaa56488f8ae678e47661e861e8e0c6db8e2a8/third_party/WebKit/Source/bindings/core/v8/V8PrivateProperty.h

Project Member

Comment 13 by bugdroid1@chromium.org, Apr 10 2017

Sign in to add a comment