[webkit-reviews] review granted: [Bug 194935] [JSC] putNonEnumerable in JSWrapperMap is too costly : [Attachment 362698] Patch

[bugzilla-daemon at webkit.org]

Mark Lam <mark.lam at apple.com> has granted Yusuke Suzuki <ysuzuki at apple.com>'s
request for review:
Bug 194935: [JSC] putNonEnumerable in JSWrapperMap is too costly
https://bugs.webkit.org/show_bug.cgi?id=194935

Attachment 362698: Patch

https://bugs.webkit.org/attachment.cgi?id=362698&action=review




--- Comment #4 from Mark Lam <mark.lam at apple.com> ---
Comment on attachment 362698
  --> https://bugs.webkit.org/attachment.cgi?id=362698
Patch

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

r=me with ChangeLog clarification.

> Source/JavaScriptCore/ChangeLog:8
> +	   When we convert Objective-C blocks to JS objects, we need to set up
corresponding function object correctly.

set up *a* corresponding ...

> Source/JavaScriptCore/ChangeLog:10
> +	   The problem is that this API has particularly costly implementation.

has *a* particularly ...

I also suggest terminating the sentence with a ':' instead of a '.' to indicate
that the next line is related to the "costly implementation" that we spoke of
here.

> Source/JavaScriptCore/ChangeLog:17
> +	   This wraps each JS objects appear in this code with Objective-C
wrapper. And we convert a NSDictionary to JSObject, which
> +	   has "writable", "enumerable", "configurable", "value" fields, and
call the "defineProperty" JS function through Objective-C wrapper.
> +	   This allocates many Objective-C wrappers and JS objects for
descriptors. Since JSC has a direct C++ API "defineOwnProperty", we should
> +	   bypass these Objective-C APIs and call JSC's code directly.

Can you clarify what you meant by "This wraps each JS objects appear in this
code with Objective-C wrapper"?

> Source/JavaScriptCore/ChangeLog:22
> +	   except for this (converting an Objective-C block to a JS object) one
path. And (2) even though [JSValue defineProperty:descriptor] is rewritten,
> +	   we still want to call JSC C++ code directly here to avoid
NSDictionary allocation for a descriptor.

I suggest rephrasing this as "even if we were to re-write [JSValue
defineProperty:descriptor] to be more optimized, we would still want to call
the JSC c++ version of defineProperty directly here ..."

> Source/JavaScriptCore/API/JSWrapperMap.mm:187
> +    JSC::JSLockHolder locker(exec);

Let's acquire the locker using the JSLockHolder(VM&) form.  It's more efficient
especially since you also need to compute vm here.