Bug 156116 - We should support the ability to do a non-effectful getById
Summary: We should support the ability to do a non-effectful getById
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Keith Miller
URL:
Keywords:
Depends on: 156261 156301
Blocks: 173305
  Show dependency treegraph
 
Reported: 2016-04-01 14:01 PDT by Keith Miller
Modified: 2017-06-13 09:29 PDT (History)
4 users (show)

See Also:


Attachments
Patch (57.05 KB, patch)
2016-04-01 14:52 PDT, Keith Miller
no flags Details | Formatted Diff | Diff
Patch (60.90 KB, patch)
2016-04-01 15:32 PDT, Keith Miller
no flags Details | Formatted Diff | Diff
Patch (63.77 KB, patch)
2016-04-04 15:47 PDT, Keith Miller
no flags Details | Formatted Diff | Diff
Patch for landing (83.27 KB, patch)
2016-04-05 13:40 PDT, Keith Miller
no flags Details | Formatted Diff | Diff
Patch for landing (83.42 KB, patch)
2016-04-05 14:09 PDT, Keith Miller
no flags Details | Formatted Diff | Diff
Patch for landing (63.73 KB, patch)
2016-04-06 09:44 PDT, Keith Miller
no flags Details | Formatted Diff | Diff
Patch for landing (72.70 KB, patch)
2016-04-07 11:59 PDT, Keith Miller
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Keith Miller 2016-04-01 14:01:49 PDT
We should support the ability to do a pure getById
Comment 1 Keith Miller 2016-04-01 14:52:24 PDT
Created attachment 275430 [details]
Patch
Comment 2 WebKit Commit Bot 2016-04-01 14:55:01 PDT
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.
Comment 3 Keith Miller 2016-04-01 15:32:10 PDT
Created attachment 275437 [details]
Patch
Comment 4 WebKit Commit Bot 2016-04-01 15:33:33 PDT
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.
Comment 5 Keith Miller 2016-04-01 16:25:21 PDT
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.
Comment 6 Keith Miller 2016-04-04 15:47:44 PDT
Created attachment 275584 [details]
Patch
Comment 7 WebKit Commit Bot 2016-04-04 15:50:13 PDT
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.
Comment 8 Benjamin Poulain 2016-04-04 17:23:58 PDT
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.
Comment 9 Keith Miller 2016-04-05 09:21:36 PDT
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.
Comment 10 Mark Lam 2016-04-05 10:49:37 PDT
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/.
Comment 11 Keith Miller 2016-04-05 13:40:24 PDT
Created attachment 275688 [details]
Patch for landing
Comment 12 WebKit Commit Bot 2016-04-05 13:42:26 PDT
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
Comment 13 Keith Miller 2016-04-05 14:09:09 PDT
Created attachment 275693 [details]
Patch for landing
Comment 14 Keith Miller 2016-04-05 14:53:13 PDT
This landed in <https://trac.webkit.org/changeset/199073>
Comment 15 WebKit Commit Bot 2016-04-05 16:48:18 PDT
Re-opened since this is blocked by bug 156261
Comment 16 Keith Miller 2016-04-06 09:44:34 PDT
Created attachment 275776 [details]
Patch for landing
Comment 17 Keith Miller 2016-04-06 09:47:40 PDT
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.
Comment 18 WebKit Commit Bot 2016-04-06 09:54:17 PDT
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.
Comment 19 WebKit Commit Bot 2016-04-06 10:36:14 PDT
Comment on attachment 275776 [details]
Patch for landing

Clearing flags on attachment: 275776

Committed r199104: <http://trac.webkit.org/changeset/199104>
Comment 20 WebKit Commit Bot 2016-04-06 10:36:19 PDT
All reviewed patches have been landed.  Closing bug.
Comment 21 WebKit Commit Bot 2016-04-06 11:45:57 PDT
Re-opened since this is blocked by bug 156301
Comment 22 Keith Miller 2016-04-07 11:59:55 PDT
Created attachment 275907 [details]
Patch for landing
Comment 23 WebKit Commit Bot 2016-04-07 12:02:33 PDT
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.
Comment 24 WebKit Commit Bot 2016-04-07 12:38:02 PDT
Comment on attachment 275907 [details]
Patch for landing

Clearing flags on attachment: 275907

Committed r199170: <http://trac.webkit.org/changeset/199170>
Comment 25 WebKit Commit Bot 2016-04-07 12:38:07 PDT
All reviewed patches have been landed.  Closing bug.