Bug 136983 - Structure should have a method for concurrently getting all of the property map entries, and this method shouldn't involve copy-paste
Summary: Structure should have a method for concurrently getting all of the property m...
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:
Blocks: 136330
  Show dependency tree / graph
 
Reported: 2014-09-21 12:56 PDT by Filip Pizlo
Modified: 2014-09-21 16:53 PDT (History)
10 users (show)

See Also:

Attachments
the patch (8.59 KB, patch)
2014-09-21 13:01 PDT, Filip Pizlo 		no flags Details | Formatted Diff | Diff
the patch (8.66 KB, patch)
2014-09-21 13:20 PDT, Filip Pizlo 		mhahnenb: review+
Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Filip Pizlo 2014-09-21 12:56:18 PDT 
Patch forthcoming.
Comment 1 Filip Pizlo 2014-09-21 13:01:06 PDT 
Created attachment 238429 [details]
the patch
Comment 2 WebKit Commit Bot 2014-09-21 13:03:53 PDT 
Attachment 238429 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/Structure.cpp:1115:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Filip Pizlo 2014-09-21 13:20:13 PDT 
Created attachment 238431 [details]
the patch

For real this time.
Comment 4 Mark Hahnenberg 2014-09-21 14:10:04 PDT 
Comment on attachment 238431 [details]
the patch

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

r=me with comments.

> Source/JavaScriptCore/runtime/Structure.h:76
> +struct PropertyMapEntry {

Maybe it makes sense to just pull this into its own header? It seems sort of weird to move it to Structure.h from PropertyMapHashTable.h.

> Source/JavaScriptCore/runtime/StructureInlines.h:117
> +        for (auto entry : *table) {

auto& maybe? Do we want to copy each entry as we're iterating?
Comment 5 Filip Pizlo 2014-09-21 16:50:37 PDT 
(In reply to comment #4)
> (From update of attachment 238431 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=238431&action=review
> 
> r=me with comments.
> 
> > Source/JavaScriptCore/runtime/Structure.h:76
> > +struct PropertyMapEntry {
> 
> Maybe it makes sense to just pull this into its own header? It seems sort of weird to move it to Structure.h from PropertyMapHashTable.h.

I would love to be able to do that.  Structure.h is included by everything in WebKit, so moving PropertyMapEntry into another head and including it from Structure.h would mean that there would be one more header that is included by everything in WebKit.  Last I checked, such things make kling and smfr sad.

> 
> > Source/JavaScriptCore/runtime/StructureInlines.h:117
> > +        for (auto entry : *table) {
> 
> auto& maybe? Do we want to copy each entry as we're iterating?

Lol OK.  In this case it can't possibly matter.  The body of the loop will use the elements on entry.  If we use auto without the & it means that initially clang will emit loads for all of the elements of PropertyMapEntry, which are all PODs so these are really just simple loads, and then llvm will DCE the loads that you didn't use.  If you use auto&, then clang will emit the loads at the point of use.  It doesn't matter.
Comment 6 Filip Pizlo 2014-09-21 16:53:01 PDT 
Landed in http://trac.webkit.org/changeset/173799