Bug 119277 – Reduce JSC API static value setter/getter overhead

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Yi Shen]

To set/get the static value, JSC APIs always call OpaqueJSString::create() to make a thread-safe string copy and pass it to the setXXX/getXXX function which is implemented by the API client. However, the client's setXXX/getXXX doesn't need to know the property name in most case (because they already know the property name). To reduce the overhead, it would be nice to introduce a new JSPropertyAttribute to allow the client decide whether a property name string copy should be created and passed as a parameter.

[Yi Shen]

Created attachment 207769 [details]
Test case

[Yi Shen]

Created attachment 207770 [details]
proposal patch

[WebKit Commit Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/API/JSCallbackObjectFunctions.h', u'Source/JavaScriptCore/API/JSObjectRef.h', u'Source/JavaScriptCore/ChangeLog']" exit_code: 1
Source/JavaScriptCore/API/JSObjectRef.h:55:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Source/JavaScriptCore/API/JSObjectRef.h:56:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 2 in 3 files


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

[Yi Shen]

(In reply to comment #3)
> Attachment 207770 [details] did not pass style-queue:
> 
> Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/API/JSCallbackObjectFunctions.h', u'Source/JavaScriptCore/API/JSObjectRef.h', u'Source/JavaScriptCore/ChangeLog']" exit_code: 1
> Source/JavaScriptCore/API/JSObjectRef.h:55:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
> Source/JavaScriptCore/API/JSObjectRef.h:56:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
> Total errors found: 2 in 3 files
> 
> 
> If any of these errors are false positives, please file a bug against check-webkit-style.

The existing & new added JSPropertyAttribute are not using InterCaps with and initial capital letter, which makes the style checker unhappy. But I think we should keep that.

[Yi Shen]

result(ms) 
                  Pass name          Don't pass name
                 getter/setter          getter/setter
1.                 344/321               193/159
2.                 375/341               196/159
3.                 337/322               193/160
4.                 339/317               193/158
5.                 342/323               195/159
6.                 338/320               194/159
7.                 338/322               195/160
8.                 341/333               193/159
9.                 355/323               195/161
10.               354/337               195/157
----------------------------------------------------
avg               346/326               194/159

[Geoffrey Garen]

Comment on attachment 207770 [details]
proposal patch

I don't think we should fix this with a special attribute. What about all our existing clients that don't use the attribute?

Let's optimize this path not to call OpaqueJSString::create() every time.

[Yi Shen]

Created attachment 207866 [details]
avoid to create OpaqueJSString every time

[Yi Shen]

Thanks for reviewing. I added a new patch based on your comments.
(In reply to comment #6)
> (From update of attachment 207770 [details])
> I don't think we should fix this with a special attribute. What about all our existing clients that don't use the attribute?
> 
> Let's optimize this path not to call OpaqueJSString::create() every time.

[EFL EWS Bot]

Comment on attachment 207866 [details]
avoid to create OpaqueJSString every time

Attachment 207866 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/1288847

[Geoffrey Garen]

Comment on attachment 207866 [details]
avoid to create OpaqueJSString every time

This looks good, but it leaves the code in a bit of a confused state, because now getters build in a propertyNameRef, but setters still call "propertyNameRef = OpaqueJSString::create(name);", and it's not clear why. Can you update setters to match getters as well?

[Yi Shen]

Thanks for reviewing :)

The setters call "OpaqueJSString::create()" only if there is a setProperty function defined in the callee JSClass (OpaqueJSClass::setProperty). In this case, the property name is undetermined until the setters was invoked (Actually, user can pass any value as a property name and client's code will determine whether it is valid). One solution to avoid to create OpaqueJSString every time in this case is to use a Map structure, but it will cause memory overhead.

For JSClass that doesn't have a setProperty function, the setters search the static values table by property name and call the set function only if it can find. My patch only focus on this case, and has updated setters to match the static value getters. Please check the changes for JSCallbackObject<Parent>::put() and JSCallbackObject<Parent>::putByIndex. 

(In reply to comment #10)
> (From update of attachment 207866 [details])
> This looks good, but it leaves the code in a bit of a confused state, because now getters build in a propertyNameRef, but setters still call "propertyNameRef = OpaqueJSString::create(name);", and it's not clear why. Can you update setters to match getters as well?

[Yi Shen]

Created attachment 207878 [details]
add comments to eliminate confusion

[EFL EWS Bot]

Comment on attachment 207878 [details]
add comments to eliminate confusion

Attachment 207878 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/1300718

[Geoffrey Garen]

Sorry, you were right the first time -- I just misread the code.

[Geoffrey Garen]

Comment on attachment 207866 [details]
avoid to create OpaqueJSString every time

r=me

[Geoffrey Garen]

Comment on attachment 207878 [details]
add comments to eliminate confusion

This version isn't necessary.

[WebKit Commit Bot]

Comment on attachment 207866 [details]
avoid to create OpaqueJSString every time

Clearing flags on attachment: 207866

Committed r153547: <http://trac.webkit.org/changeset/153547>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Yi Shen]

Thank you, Geoffrey :]
(In reply to comment #15)
> (From update of attachment 207866 [details])
> r=me