[webkit-reviews] review denied: [Bug 25175] It is possible for unique anonymous functions to hash to the same value, thus showing up as one function in the profiler : [Attachment 29474] Changed JSValuePtr to JSObject*

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Francisco Tolmasky
<tolmasky at gmail.com>'s request for review:
Bug 25175: It is possible for unique anonymous functions to hash to the same
value, thus showing up as one function in the profiler
https://bugs.webkit.org/show_bug.cgi?id=25175

Attachment 29474: Changed JSValuePtr to JSObject*
https://bugs.webkit.org/attachment.cgi?id=29474&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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