Bug 119417 - Use op_get_by_id_custom_stub to fast access to JSCallbackObject's static value
Summary: Use op_get_by_id_custom_stub to fast access to JSCallbackObject's static value
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-08-01 17:35 PDT by Yi Shen
Modified: 2013-10-31 13:52 PDT (History)
2 users (show)

See Also:


Attachments
Test case (355 bytes, text/html)
2013-08-01 17:35 PDT, Yi Shen
no flags Details
proposal patch (4.03 KB, patch)
2013-08-01 17:41 PDT, Yi Shen
ggaren: review-
Details | Formatted Diff | Diff
updated with test (15.02 KB, patch)
2013-08-13 15:06 PDT, Yi Shen
ggaren: review-
Details | Formatted Diff | Diff
updated patch (16.32 KB, patch)
2013-08-29 11:44 PDT, Yi Shen
oliver: review+
commit-queue: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yi Shen 2013-08-01 17:35:00 PDT
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).
Comment 1 Yi Shen 2013-08-01 17:41:57 PDT
Created attachment 207971 [details]
proposal patch
Comment 2 Yi Shen 2013-08-01 17:47:49 PDT
               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
Comment 3 Geoffrey Garen 2013-08-01 17:50:31 PDT
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.
Comment 4 Yi Shen 2013-08-02 09:39:53 PDT
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.
Comment 5 Yi Shen 2013-08-13 15:06:19 PDT
Created attachment 208682 [details]
updated with test
Comment 6 WebKit Commit Bot 2013-08-13 15:09:32 PDT
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.
Comment 7 Yi Shen 2013-08-21 16:15:52 PDT
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.
Comment 8 Geoffrey Garen 2013-08-28 17:52:10 PDT
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.
Comment 9 Yi Shen 2013-08-29 11:42:35 PDT
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.
Comment 10 Yi Shen 2013-08-29 11:44:34 PDT
Created attachment 210008 [details]
updated patch
Comment 11 WebKit Commit Bot 2013-08-29 11:51:14 PDT
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.
Comment 12 Oliver Hunt 2013-10-31 13:51:37 PDT
Comment on attachment 210008 [details]
updated patch

It seems like we should be able to do this for static functions as well.
Comment 13 WebKit Commit Bot 2013-10-31 13:52:48 PDT
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