Bug 156116 – We should support the ability to do a non-effectful getById

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Keith Miller]

We should support the ability to do a pure getById

[Keith Miller]

Created attachment 275430 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.h:99:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 37 files


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

[Keith Miller]

Created attachment 275437 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.h:99:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 39 files


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

[Keith Miller]

Comment on attachment 275437 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:24
> +        likely to have little to no impact on memory usage as normal accessors are generally rare.

I will add a comment in here that this patch only adds baseline support and DFG support will come later before landing.

[Keith Miller]

Created attachment 275584 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.h:99:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 40 files


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

[Benjamin Poulain]

Comment on attachment 275584 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:8
> +        Currently, there is no way in JS to do a pure getById. A pure getById is useful becasue it

Typo: becasue

> Source/JavaScriptCore/builtins/BuiltinExecutables.h:55
> +    // This function is exposed for testing purposes only.

I don't think that comment is necessary.
It is a pretty common use of EXPORT.

> Source/JavaScriptCore/bytecode/PolymorphicAccess.cpp:157
> +    const ObjectPropertyConditionSet& conditionSet,  bool viaProxy, WatchpointSet* additionalSet)

Two space after the comma.

[Keith Miller]

Comment on attachment 275584 [details]
Patch

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

>> Source/JavaScriptCore/ChangeLog:8
>> +        Currently, there is no way in JS to do a pure getById. A pure getById is useful becasue it
> 
> Typo: becasue

Fixed.

>> Source/JavaScriptCore/builtins/BuiltinExecutables.h:55
>> +    // This function is exposed for testing purposes only.
> 
> I don't think that comment is necessary.
> It is a pretty common use of EXPORT.

I guess that's fair, I've removed the comment.

>> Source/JavaScriptCore/bytecode/PolymorphicAccess.cpp:157
>> +    const ObjectPropertyConditionSet& conditionSet,  bool viaProxy, WatchpointSet* additionalSet)
> 
> Two space after the comma.

Fixed.

[Mark Lam]

Comment on attachment 275584 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:14
> +        undefined if the slot is unset.  If the slot is proxied or any other cases then the result

What did you mean by "or any other cases" here?

> Source/JavaScriptCore/ChangeLog:23
> +        with === GetterSetters are now JSObjects. This comes at the cost of one extra byte on the

Perhaps a ',' after the '===' will make this sentence clearer.  Also, I think you mean "one extra word" instead of "one extra byte".

> Source/JavaScriptCore/ChangeLog:24
> +        GetterSetter object but it vastly simplifies the patch. Additionally, the extra byte is

Ditto: /byte/word/.

[Keith Miller]

Created attachment 275688 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 275688 [details]
Patch for landing

Rejecting attachment 275688 [details] from commit-queue.

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

Last 500 characters of output:
GetterSetter.cpp
patching file Source/JavaScriptCore/runtime/GetterSetter.h
patching file Source/JavaScriptCore/runtime/JSType.h
patching file Source/JavaScriptCore/runtime/PropertySlot.cpp
patching file Source/JavaScriptCore/runtime/PropertySlot.h
patching file Source/JavaScriptCore/runtime/ProxyObject.cpp
patching file Source/JavaScriptCore/tests/stress/try-get-by-id.js

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

Full output: http://webkit-queues.webkit.org/results/1105360

[Keith Miller]

Created attachment 275693 [details]
Patch for landing

[Keith Miller]

This landed in <https://trac.webkit.org/changeset/199073>

[WebKit Commit Bot]

Re-opened since this is blocked by bug 156261

[Keith Miller]

Created attachment 275776 [details]
Patch for landing

[Keith Miller]

I guess the last patch changed a bunch of xcodebuild files. Hopefully, undoing those changes unbreaks some of the build issues. I was unable to get the build problems to reproduce locally but I expect that is the only thing in the patch that could cause the breakage.

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.h:99:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 40 files


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

[WebKit Commit Bot]

Comment on attachment 275776 [details]
Patch for landing

Clearing flags on attachment: 275776

Committed r199104: <http://trac.webkit.org/changeset/199104>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 156301

[Keith Miller]

Created attachment 275907 [details]
Patch for landing

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.h:99:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 43 files


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

[WebKit Commit Bot]

Comment on attachment 275907 [details]
Patch for landing

Clearing flags on attachment: 275907

Committed r199170: <http://trac.webkit.org/changeset/199170>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.