Bug 119417 – Use op_get_by_id_custom_stub to fast access to JSCallbackObject's static value

• Status: NEW
• Component: JavaScriptCore

[Yi Shen]

Created attachment 207970 [details]
Test case

To access to JSCallbackObject's static value, JSC API needs to call JSCallbackObject::getOwnPropertySlot() first, then invokes the JSCallbackObject::getStaticValue(). The work done in getOwnPropertySlot() could be saved if we can use Baseline JIT stub (op_get_by_id_custom_stub) to call getStaticValue() directly when Baseline JIT kicks in (e.g. property access in a loop which takes 100 times). It surely can speed up the client applications that need heavy property accesses in their code. (e.g. a game app's draw function).

[Yi Shen]

Created attachment 207971 [details]
proposal patch

[Yi Shen]

               No JIT stub     JIT stub
result(ms)
1.                  199          157
2.                  200          155
3.                  198          157
4.                  198          157
5.                  199          158
6.                  198          158
7.                  198          157
8.                  197          157
9.                  196          156
10.                 197          156
---------------------------------------
avg                 198        157

---------------------------------------------
                                            No JIT stub        JIT stub
invoked times 
1.getOwnPropertySlot()          1000000            100
2.getStaticValue()                   1000000            1000000

[Geoffrey Garen]

Comment on attachment 207971 [details]
proposal patch

This isn't quite right.

The API allows a client to specify an abstract hasProperty and/or getProperty callback, and that callback is allowed to return different results every time, making it incorrect to cache.

Please add a test to testapi that fails for this reason with your current patch.

That said, you may be able to rescue the basic idea of this optimization: You just need to modify your patch to be sure to poison all property access as uncacheable if anything in the class or prototype hierarchy specifies an abstract getProperty or hasProperty callback.

[Yi Shen]

Thanks! I will update my patch with the test.
(In reply to comment #3)
> (From update of attachment 207971 [details])
> This isn't quite right.
> 
> The API allows a client to specify an abstract hasProperty and/or getProperty callback, and that callback is allowed to return different results every time, making it incorrect to cache.
> 
> Please add a test to testapi that fails for this reason with your current patch.
> 
> That said, you may be able to rescue the basic idea of this optimization: You just need to modify your patch to be sure to poison all property access as uncacheable if anything in the class or prototype hierarchy specifies an abstract getProperty or hasProperty callback.

[Yi Shen]

Created attachment 208682 [details]
updated with test

[WebKit Commit Bot]

Attachment 208682 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/API/JSCallbackObject.h', u'Source/JavaScriptCore/API/JSCallbackObjectFunctions.h', u'Source/JavaScriptCore/API/JSClassRef.cpp', u'Source/JavaScriptCore/API/JSClassRef.h', u'Source/JavaScriptCore/API/tests/testapi.c', u'Source/JavaScriptCore/API/tests/testapi.js', u'Source/JavaScriptCore/ChangeLog', u'Source/JavaScriptCore/runtime/PropertySlot.h', u'Source/JavaScriptCore/runtime/PutPropertySlot.h']" exit_code: 1
Source/JavaScriptCore/API/tests/testapi.c:678:  MyCacheableObject_get_cacheableStaticValue is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/JavaScriptCore/API/tests/testapi.c:694:  CacheableObject_definition is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/JavaScriptCore/API/tests/testapi.c:717:  MyCacheableObject_class is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/JavaScriptCore/API/tests/testapi.c:728:  MyUncacheableObject_getProperty is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/JavaScriptCore/API/tests/testapi.c:744:  MyUncacheableObject_get_uncacheableStaticValue is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/JavaScriptCore/API/tests/testapi.c:758:  UncacheableObject_definition is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/JavaScriptCore/API/tests/testapi.c:781:  MyUncacheableObject_class is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 7 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yi Shen]

should we skip the style check for the API/tests/testapi.c ?
(In reply to comment #6)
> Attachment 208682 [details] did not pass style-queue:
> 
> Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/API/JSCallbackObject.h', u'Source/JavaScriptCore/API/JSCallbackObjectFunctions.h', u'Source/JavaScriptCore/API/JSClassRef.cpp', u'Source/JavaScriptCore/API/JSClassRef.h', u'Source/JavaScriptCore/API/tests/testapi.c', u'Source/JavaScriptCore/API/tests/testapi.js', u'Source/JavaScriptCore/ChangeLog', u'Source/JavaScriptCore/runtime/PropertySlot.h', u'Source/JavaScriptCore/runtime/PutPropertySlot.h']" exit_code: 1
> Source/JavaScriptCore/API/tests/testapi.c:678:  MyCacheableObject_get_cacheableStaticValue is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
> Source/JavaScriptCore/API/tests/testapi.c:694:  CacheableObject_definition is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
> Source/JavaScriptCore/API/tests/testapi.c:717:  MyCacheableObject_class is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
> Source/JavaScriptCore/API/tests/testapi.c:728:  MyUncacheableObject_getProperty is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
> Source/JavaScriptCore/API/tests/testapi.c:744:  MyUncacheableObject_get_uncacheableStaticValue is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
> Source/JavaScriptCore/API/tests/testapi.c:758:  UncacheableObject_definition is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
> Source/JavaScriptCore/API/tests/testapi.c:781:  MyUncacheableObject_class is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
> Total errors found: 7 in 9 files
> 
> 
> If any of these errors are false positives, please file a bug against check-webkit-style.

[Geoffrey Garen]

Comment on attachment 208682 [details]
updated with test

View in context: https://bugs.webkit.org/attachment.cgi?id=208682&action=review

I think this approach will work, but I'd like to see some refinement.

> Source/JavaScriptCore/API/JSCallbackObjectFunctions.h:172
> +                    JSValue value = getStaticValue(exec, thisObject, propertyName);
>                      if (value) {
> +                        if (thisObject->classRef()->isCacheable())
> +                            entry->cacheable = true;

I'd prefer to see the cacheable field initialized when the StaticValueEntry was created. You can do this in OpaqueJSClassContextData::OpaqueJSClassContextData().

> Source/JavaScriptCore/API/JSCallbackObjectFunctions.h:189
> +    slot.setUnCacheable(); // Don't cache Parent's property

I don't think this comment adds anything that wasn't there in code.

Also, we should only do this if our class is uncacheable, right? We don't want to make all parent class properties unconditionally uncacheable.

> Source/JavaScriptCore/API/JSCallbackObjectFunctions.h:301
> +    Parent::put(thisObject, exec, propertyName, value, slot);
> +    slot.setUnCacheable(); // Don't cache Parent's property
> +    return;

Ditto.

> Source/JavaScriptCore/API/JSClassRef.h:48
> +    bool cacheable;

Let's call this "isCacheable".

> Source/JavaScriptCore/API/JSClassRef.h:127
> +    bool m_cacheable;

Let's call this "m_isCacheable".

> Source/JavaScriptCore/runtime/PropertySlot.h:64
> +    void setUnCacheable() { m_offset = invalidOffset; }

"uncacheable" is one word, so let's call this "setUncacheable".

> Source/JavaScriptCore/runtime/PutPropertySlot.h:68
> +        void setUnCacheable() { m_type = Uncachable; }

Ditto.

[Yi Shen]

Thanks a lot! Will add a new patch based on your comments.
(In reply to comment #8)
> (From update of attachment 208682 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=208682&action=review
> 
> I think this approach will work, but I'd like to see some refinement.
> 
> > Source/JavaScriptCore/API/JSCallbackObjectFunctions.h:172
> > +                    JSValue value = getStaticValue(exec, thisObject, propertyName);
> >                      if (value) {
> > +                        if (thisObject->classRef()->isCacheable())
> > +                            entry->cacheable = true;
> 
> I'd prefer to see the cacheable field initialized when the StaticValueEntry was created. You can do this in OpaqueJSClassContextData::OpaqueJSClassContextData().
> 
> > Source/JavaScriptCore/API/JSCallbackObjectFunctions.h:189
> > +    slot.setUnCacheable(); // Don't cache Parent's property
> 
> I don't think this comment adds anything that wasn't there in code.
> 
> Also, we should only do this if our class is uncacheable, right? We don't want to make all parent class properties unconditionally uncacheable.
> 
> > Source/JavaScriptCore/API/JSCallbackObjectFunctions.h:301
> > +    Parent::put(thisObject, exec, propertyName, value, slot);
> > +    slot.setUnCacheable(); // Don't cache Parent's property
> > +    return;
> 
> Ditto.
> 
> > Source/JavaScriptCore/API/JSClassRef.h:48
> > +    bool cacheable;
> 
> Let's call this "isCacheable".
> 
> > Source/JavaScriptCore/API/JSClassRef.h:127
> > +    bool m_cacheable;
> 
> Let's call this "m_isCacheable".
> 
> > Source/JavaScriptCore/runtime/PropertySlot.h:64
> > +    void setUnCacheable() { m_offset = invalidOffset; }
> 
> "uncacheable" is one word, so let's call this "setUncacheable".
> 
> > Source/JavaScriptCore/runtime/PutPropertySlot.h:68
> > +        void setUnCacheable() { m_type = Uncachable; }
> 
> Ditto.

[Yi Shen]

Created attachment 210008 [details]
updated patch

[WebKit Commit Bot]

Attachment 210008 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/API/JSCallbackObject.h', u'Source/JavaScriptCore/API/JSCallbackObjectFunctions.h', u'Source/JavaScriptCore/API/JSClassRef.cpp', u'Source/JavaScriptCore/API/JSClassRef.h', u'Source/JavaScriptCore/API/tests/testapi.c', u'Source/JavaScriptCore/API/tests/testapi.js', u'Source/JavaScriptCore/ChangeLog', u'Source/JavaScriptCore/runtime/PropertySlot.h', u'Source/JavaScriptCore/runtime/PutPropertySlot.h']" exit_code: 1
Source/JavaScriptCore/API/tests/testapi.c:678:  MyCacheableObject_get_cacheableStaticValue is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/JavaScriptCore/API/tests/testapi.c:694:  CacheableObject_definition is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/JavaScriptCore/API/tests/testapi.c:717:  MyCacheableObject_class is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/JavaScriptCore/API/tests/testapi.c:728:  MyUncacheableObject_getProperty is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/JavaScriptCore/API/tests/testapi.c:744:  MyUncacheableObject_get_uncacheableStaticValue is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/JavaScriptCore/API/tests/testapi.c:758:  UncacheableObject_definition is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/JavaScriptCore/API/tests/testapi.c:781:  MyUncacheableObject_class is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 7 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Oliver Hunt]

Comment on attachment 210008 [details]
updated patch

It seems like we should be able to do this for static functions as well.

[WebKit Commit Bot]

Comment on attachment 210008 [details]
updated patch

Rejecting attachment 210008 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-01', 'apply-attachment', '--no-update', '--non-interactive', 210008, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
k #3 succeeded at 1436 (offset 38 lines).
patching file Source/JavaScriptCore/API/tests/testapi.js
patching file Source/JavaScriptCore/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file Source/JavaScriptCore/runtime/PropertySlot.h
Hunk #1 succeeded at 74 (offset -1 lines).
patching file Source/JavaScriptCore/runtime/PutPropertySlot.h

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Oliver Hunt']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Full output: http://webkit-queues.appspot.com/results/18808037