Summary: | We should support the ability to do a non-effectful getById | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Keith Miller <keith_miller> | ||||||||||||||||
Component: | New Bugs | Assignee: | Keith Miller <keith_miller> | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Normal | CC: | commit-queue, mark.lam, msaboff, saam | ||||||||||||||||
Priority: | P2 | ||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||
Bug Depends on: | 156261, 156301 | ||||||||||||||||||
Bug Blocks: | 173305 | ||||||||||||||||||
Attachments: |
|
Description
Keith Miller
2016-04-01 14:01:49 PDT
Created attachment 275430 [details]
Patch
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.
Created attachment 275437 [details]
Patch
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 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. Created attachment 275584 [details]
Patch
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 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 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 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/. Created attachment 275688 [details]
Patch for landing
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 Created attachment 275693 [details]
Patch for landing
This landed in <https://trac.webkit.org/changeset/199073> Re-opened since this is blocked by bug 156261 Created attachment 275776 [details]
Patch for landing
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. 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 on attachment 275776 [details] Patch for landing Clearing flags on attachment: 275776 Committed r199104: <http://trac.webkit.org/changeset/199104> All reviewed patches have been landed. Closing bug. Re-opened since this is blocked by bug 156301 Created attachment 275907 [details]
Patch for landing
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 on attachment 275907 [details] Patch for landing Clearing flags on attachment: 275907 Committed r199170: <http://trac.webkit.org/changeset/199170> All reviewed patches have been landed. Closing bug. |