Bug 93150 - Array checks should use the structure, not the class info
Summary: Array checks should use the structure, not the class info
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Filip Pizlo
URL:
Keywords:
Depends on: 93162
Blocks: 91933
  Show dependency treegraph
 
Reported: 2012-08-03 12:18 PDT by Filip Pizlo
Modified: 2012-08-14 19:49 PDT (History)
3 users (show)

See Also:


Attachments
work in progress (26.95 KB, patch)
2012-08-03 12:22 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
more (33.86 KB, patch)
2012-08-03 18:07 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (71.52 KB, patch)
2012-08-13 23:57 PDT, Filip Pizlo
mhahnenberg: review+
buildbot: commit-queue-
Details | Formatted Diff | Diff
patch for landing, hopefully (77.51 KB, patch)
2012-08-14 18:57 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Filip Pizlo 2012-08-03 12:18:43 PDT
This implies that the LLInt and baseline JIT must profile which structure was observed, so that the DFG may pick the right structure to check on the fast path.
Comment 1 Filip Pizlo 2012-08-03 12:22:58 PDT
Created attachment 156431 [details]
work in progress
Comment 2 Filip Pizlo 2012-08-03 18:07:32 PDT
Created attachment 156494 [details]
more
Comment 3 Filip Pizlo 2012-08-13 23:57:06 PDT
Created attachment 158235 [details]
the patch
Comment 4 WebKit Review Bot 2012-08-14 00:01:39 PDT
Attachment 158235 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/CMakeLists.txt', u'S..." exit_code: 1
Source/JavaScriptCore/bytecode/Instruction.h:49:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/JavaScriptCore/bytecode/ArrayProfile.h:79:  The parameter name "operation" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 29 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 5 Build Bot 2012-08-14 01:22:36 PDT
Comment on attachment 158235 [details]
the patch

Attachment 158235 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13501104
Comment 6 Mark Hahnenberg 2012-08-14 14:32:01 PDT
Comment on attachment 158235 [details]
the patch

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

Looks good overall except for minor fix. Also placate windows and style bot please :-)

> Source/JavaScriptCore/jit/JITPropertyAccess32_64.cpp:214
> +    addSlowCase(branchPtr(NotEqual, Address(regT1, JSCell::classInfoOffset()), TrustedImmPtr(&JSArray::s_info)));

Should this be Structure::classInfoOffset()?
Comment 7 Filip Pizlo 2012-08-14 16:00:05 PDT
(In reply to comment #6)
> (From update of attachment 158235 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=158235&action=review
> 
> Looks good overall except for minor fix. Also placate windows and style bot please :-)
> 
> > Source/JavaScriptCore/jit/JITPropertyAccess32_64.cpp:214
> > +    addSlowCase(branchPtr(NotEqual, Address(regT1, JSCell::classInfoOffset()), TrustedImmPtr(&JSArray::s_info)));
> 
> Should this be Structure::classInfoOffset()?

Good catch!  Thanks!
Comment 8 Filip Pizlo 2012-08-14 18:57:34 PDT
Created attachment 158475 [details]
patch for landing, hopefully
Comment 9 WebKit Review Bot 2012-08-14 19:00:39 PDT
Attachment 158475 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/CMakeLists.txt', u'S..." exit_code: 1
Source/JavaScriptCore/bytecode/Instruction.h:49:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/JavaScriptCore/bytecode/ArrayProfile.h:79:  The parameter name "operation" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 32 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 10 Filip Pizlo 2012-08-14 19:49:26 PDT
Landed in http://trac.webkit.org/changeset/125637