Bug 107378 - Track inheritance structures in a side table, instead of using a private name in each prototype
Summary: Track inheritance structures in a side table, instead of using a private name...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Geoffrey Garen
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-01-19 13:08 PST by Geoffrey Garen
Modified: 2013-02-01 07:04 PST (History)
5 users (show)

See Also:

Attachments
Patch (29.87 KB, patch)
2013-01-19 13:26 PST, Geoffrey Garen 		ggaren: review+
Details | Formatted Diff | Diff
performance (19.07 KB, text/plain)
2013-01-19 13:28 PST, Geoffrey Garen 		no flags Details
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 Geoffrey Garen 2013-01-19 13:08:23 PST 
Track inheritance structures in a side table, instead of using a private name in each prototype
Comment 1 Geoffrey Garen 2013-01-19 13:26:15 PST 
Created attachment 183632 [details]
Patch
Comment 2 Geoffrey Garen 2013-01-19 13:28:10 PST 
Created attachment 183633 [details]
performance
Comment 3 Geoffrey Garen 2013-01-19 13:28:50 PST 
Seems performance-neutral, as expected.
Comment 4 Geoffrey Garen 2013-01-19 13:48:47 PST 
Comment on attachment 183632 [details]
Patch

Sam and Phil reviewed this.
Comment 5 Geoffrey Garen 2013-01-19 13:49:02 PST 
Committed r140259: <http://trac.webkit.org/changeset/140259>
Comment 7 Geoffrey Garen 2013-01-20 12:35:10 PST 
Disabled the ASSERT for the time being to get the bots green: <http://trac.webkit.org/changeset/140259>.
Comment 8 Geoffrey Garen 2013-01-20 17:01:24 PST 
Follow-up ASSERT fix: <http://trac.webkit.org/changeset/140284>.
Comment 9 Darin Adler 2013-01-21 10:07:14 PST 
Comment on attachment 183632 [details]
Patch

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

> Source/JavaScriptCore/runtime/PrototypeMap.h:41
> +    bool isPrototype(JSObject*); // Returns a conservative estimate.

Two thoughts:

1) If it’s a conservative estimate, then how about a name that reflects that? Perhaps mayBePrototype? I’m not really sure what a conservative estimate means.

2) Should be a const member function.
Comment 10 Filip Pizlo 2013-01-21 10:58:22 PST 
(In reply to comment #9)
> (From update of attachment 183632 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=183632&action=review
> 
> > Source/JavaScriptCore/runtime/PrototypeMap.h:41
> > +    bool isPrototype(JSObject*); // Returns a conservative estimate.
> 
> Two thoughts:
> 
> 1) If it’s a conservative estimate, then how about a name that reflects that? Perhaps mayBePrototype? I’m not really sure what a conservative estimate means.

I like mayBePrototype. But I think the reason why this was landed as isPrototype is that we were thinking of converting all of these possibly-conservative Boolean queries to use TriState. Hence you would say isPrototype(thing) != FalseTriState. 

But this would require moving the declaration of TriState from WebCore into WTF, so that'll be a separate patch. 

This TriState approach is great because I think it more accurately captures other forms of queries in JSC. It's common for us to be able to answer a question with "maybe". 

> 
> 2) Should be a const member function.
Comment 11 Geoffrey Garen 2013-02-01 07:04:13 PST 
> This TriState approach is great because I think it more accurately captures other forms of queries in JSC. It's common for us to be able to answer a question with "maybe". 

I did this in bug 108628.

> I’m not really sure what a conservative estimate means.

Added a comment to try to clarify this.

> 2) Should be a const member function.

Fixed in bug 108628.