[webkit-reviews] review denied: [Bug 186579] Share structure across instances of classes exported through the ObjC API : [Attachment 342610] Patch

[bugzilla-daemon at webkit.org]

Keith Miller <keith_miller at apple.com> has denied Tadeu Zagallo
<tzagallo at apple.com>'s request for review:
Bug 186579: Share structure across instances of classes exported through the
ObjC API
https://bugs.webkit.org/show_bug.cgi?id=186579

Attachment 342610: Patch

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




--- Comment #5 from Keith Miller <keith_miller at apple.com> ---
Comment on attachment 342610
  --> https://bugs.webkit.org/attachment.cgi?id=342610
Patch

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

r- since I don't think one of the changes is warranted but other than that it
looks good. It would also be good to add an API test for this: allocate like
50,000 api objects and check the number of live structures is something
reasonable (< 5000) afterwords.

> Source/JavaScriptCore/API/JSWrapperMap.mm:540
> -    if (!constructor)
> -	   constructor = [self
allocateConstructorAndPrototypeInContext:context].first;
> -    ASSERT(!!constructor);
> -    return constructor;
> +    if (constructor)
> +	   return constructor;
> +
> +    m_constructor = [self
allocateConstructorAndPrototypeInContext:context].first;
> +    ASSERT(!!m_constructor);
> +    return m_constructor.get();

Why make this change? allocateConstructorAndPrototypeInContext sets
m_constructor/m_prototype so you shouldn't need to do it here.

> Source/JavaScriptCore/API/JSWrapperMap.mm:551
> -    if (!prototype)
> -	   prototype = [self
allocateConstructorAndPrototypeInContext:context].second;
> -    ASSERT(!!prototype);
> -    return prototype;
> +    if (prototype)
> +	   return prototype;
> +
> +    m_prototype = [self
allocateConstructorAndPrototypeInContext:context].second;
> +    ASSERT(!!m_prototype);
> +    return m_prototype.get();

Ditto.