[webkit-reviews] review granted: [Bug 206365] Separate storage of Structure::m_offset into transition and max offset : [Attachment 387988] Patch

[bugzilla-daemon at webkit.org]

Saam Barati <sbarati at apple.com> has granted Justin Michaud
<justin_michaud at apple.com>'s request for review:
Bug 206365: Separate storage of Structure::m_offset into transition and max
offset
https://bugs.webkit.org/show_bug.cgi?id=206365

Attachment 387988: Patch

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




--- Comment #9 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 387988
  --> https://bugs.webkit.org/attachment.cgi?id=387988
Patch

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

> Source/JavaScriptCore/ChangeLog:9
> +	   assume that the transition offset (m_offset) is monotonically
increasing. In order to support structure transitions for deletion that 

we should create a bug for that work and link it here for context.

> Source/JavaScriptCore/ChangeLog:10
> +	   do not involve turning into a dictionary, we first need to separate
the transition offset that was added/deleted from the maximum offset.

you should provide context about what these fields mean, and what they're used
for

> Source/JavaScriptCore/runtime/Structure.cpp:387
> +	   RELEASE_ASSERT(i == structures.size() - 1 || structure->maxOffset()
> structures[i + 1]->maxOffset());

why add this assertion? Won't your next patch immediately make this need to be
removed? (E.g, transitions won't necessarily have this in the future).

Also, for perf reasons, might be worth making it a debug assert?

> Source/JavaScriptCore/runtime/Structure.h:393
> +	       m_transitionOffset = shortUseRareDataOffset;

why not call this "useRareDataFlag"? or similar. Not a huge fan of "short". I
kinda get it for shortInvalidOffset, but feels less needed here

> Source/JavaScriptCore/runtime/Structure.h:828
> +    static constexpr uint16_t shortUseRareDataOffset =
std::numeric_limits<uint16_t>::max()-1;

style nit: you need spaces here