Bug 25175 - It is possible for unique anonymous functions to hash to the same value, thus showing up as one function in the profiler
Summary: It is possible for unique anonymous functions to hash to the same value, thus...
Status: RESOLVED DUPLICATE of bug 20155
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-04-13 21:26 PDT by Francisco Tolmasky
Modified: 2010-02-03 09:19 PST (History)
2 users (show)

See Also:


Attachments
unique anonymous functions (903 bytes, text/html)
2009-04-13 21:28 PDT, Francisco Tolmasky
no flags Details
Added reference to actual function to tell them apart. (7.39 KB, patch)
2009-04-14 02:01 PDT, Francisco Tolmasky
oliver: review-
Details | Formatted Diff | Diff
Changed JSValuePtr to JSObject* (7.38 KB, patch)
2009-04-14 10:27 PDT, Francisco Tolmasky
no flags Details | Formatted Diff | Diff
Changed JSValuePtr to JSObject* (7.38 KB, patch)
2009-04-14 10:27 PDT, Francisco Tolmasky
darin: review-
Details | Formatted Diff | Diff
Style changes (10.59 KB, patch)
2009-04-14 23:51 PDT, Francisco Tolmasky
eric: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Francisco Tolmasky 2009-04-13 21:26:18 PDT
I am attaching a test case
Comment 1 Francisco Tolmasky 2009-04-13 21:28:53 PDT
Created attachment 29456 [details]
unique anonymous functions

You need to run this from WebKit/LayoutTests/fast/profiler/
Comment 2 Francisco Tolmasky 2009-04-13 21:39:51 PDT
This happens because functions are considered to be the "same" if they have the same name, line number, and url, which can happen for generated functions:

function create_function()
{
return function() { } // this function is unique, but has the same url, line number, and name every time it is created
}
Comment 3 Francisco Tolmasky 2009-04-14 02:01:48 PDT
Created attachment 29461 [details]
Added reference to actual function to tell them apart.
Comment 4 Oliver Hunt 2009-04-14 02:45:43 PDT
Comment on attachment 29461 [details]
Added reference to actual function to tell them apart.

The CallIdentifier should just take a JSObject* rather than the jsvalueptr
Comment 5 Francisco Tolmasky 2009-04-14 10:27:14 PDT
Created attachment 29473 [details]
Changed JSValuePtr to JSObject*
Comment 6 Francisco Tolmasky 2009-04-14 10:27:35 PDT
Created attachment 29474 [details]
Changed JSValuePtr to JSObject*
Comment 7 Darin Adler 2009-04-14 10:47:44 PDT
Comment on attachment 29474 [details]
Changed JSValuePtr to JSObject*

The function pointer should be JSFunction*, not JSObject*. We always want to use as specific a type as possible.

> +        ProtectedPtr<JSObject> m_function;

We frown on uses of ProtectedPtr, and adding a new one is truly unfortunate. Is there an easy way to eliminate this?

> -        CallIdentifier(const UString& name, const UString& url, int lineNumber)
> +        CallIdentifier(const UString& name, const UString& url, int lineNumber, JSObject * function = NULL)

WebKit project coding style for this is JSObject* function = 0.

But default arguments are confusing. Do we really need a default here?

>              return UString::Rep::computeHash(reinterpret_cast<char*>(hashCodes), sizeof(hashCodes));

This isn't new code, but it's very strange. The char* version of the computeHash function should only be used on strings.

> -            return value.m_name.isNull() && value.m_url.isNull() && value.m_lineNumber == std::numeric_limits<unsigned>::max();
> +            return value.m_name.isNull() && value.m_url.isNull() && value.m_lineNumber == std::numeric_limits<unsigned>::max() && value.m_function.get() == NULL;

WebKit coding style for this would be !value.m_function

There's no need to use == NULL or get().

> -    return CallIdentifier(name.isEmpty() ? AnonymousFunction : name, function->body()->sourceURL(), function->body()->lineNo());
> +    return CallIdentifier(name.isEmpty() ? AnonymousFunction : name, function->body()->sourceURL(), function->body()->lineNo(), asObject(function));

You don't need the asObject cast here. The asObject function is for use when downcasting, but this is upcasting and you don't need an explicit cast at all for that. But also this should be JSFunction* as I mentioned above.

Since the only goal here is to avoid aliasing, I think it's unfortunate that we are keeping the entire JSFunction object, and everything it owns, around as long as we keep the profile around. I think we might want to experiment with an alternative solution where we assign consecutive numbers to each new function and then use these unique IDs instead of actually storing a pointer to the JSFunction. But the downside of this is that it would make execution slightly slower since we'd have to store and bump this number in the JSFunction constructor. And it would make JSFunction objects bigger since we'd need a place to store the unique ID, which could make functions too large for our fixed-size-cell garbage collector. However, maybe it's not the function, but the function body, that we want to use for the unique identifier. In that case it's the FunctionBodyNode that could store the ID, and I think we could easily spare the room there. The next available unique number would be stored in JSGlobalData.

review- because of the multiple issues above
Comment 8 Oliver Hunt 2009-04-14 12:57:44 PDT
(In reply to comment #7)
> (From update of attachment 29474 [details] [review])
> The function pointer should be JSFunction*, not JSObject*. We always want to
> use as specific a type as possible.
> 
> > +        ProtectedPtr<JSObject> m_function;
It cannot be a JSFunction because being callable does not imply being a JSFunction
Comment 9 Francisco Tolmasky 2009-04-14 23:51:02 PDT
Created attachment 29491 [details]
Style changes

Made described style adjustments, kept it as JSObject * per olli's note that the object may not be a JSFunction
Comment 10 Darin Adler 2009-04-15 07:43:49 PDT
(In reply to comment #8)
> (In reply to comment #7)
> > (From update of attachment 29474 [details] [review] [review])
> > The function pointer should be JSFunction*, not JSObject*. We always want to
> > use as specific a type as possible.
> > 
> > > +        ProtectedPtr<JSObject> m_function;
> It cannot be a JSFunction because being callable does not imply being a
> JSFunction

I know that being callable does not imply JSFunction, but all the call sites in this patch *do* have a JSFunction.
Comment 11 Darin Adler 2009-04-15 07:46:05 PDT
I don't think that a ProtectedPtr<JSObject> is the right solution here. I think we want to track unique function bodies, not unique JavaScript objects being called. This should be a RefPtr<FunctionBodyNode>, not a ProtectedPtr<JSObject>, or better could use a function body ID as I suggested in my earlier comment.
Comment 12 Eric Seidel (no email) 2009-05-21 19:31:33 PDT
Comment on attachment 29491 [details]
Style changes

Neat to see Francisco contributing to WebKit!

Since there has been no response in the bug in over a month, r- based on Darin's above comment.
Comment 13 Timothy Hatcher 2010-02-03 09:19:45 PST
Looks like a dupe of bug 20155.

*** This bug has been marked as a duplicate of bug 20155 ***