It would be neat if jsSubstring() would be smarter when asked to create a substring of a JSString that is itself a substring. It should share the base string and just modify the offset instead.
Created attachment 253831 [details] Patch
Comment on attachment 253831 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=253831&action=review Yo dawg. > Source/JavaScriptCore/runtime/JSString.h:532 > + if (s->isRope() && !static_cast<JSRopeString*>(s)->isSubstring()) > + static_cast<JSRopeString*>(s)->resolveRope(exec); Can we move this branch into the callee? It feels unsafe that the callee assumes it will not be passed a non-substring rope, when nothing in its type signature guarantees it.
Comment on attachment 253831 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=253831&action=review > Source/JavaScriptCore/runtime/JSString.h:304 > + JSRopeString* baseRope = static_cast<JSRopeString*>(base); I suggest we use a reference rather than pointer here. >> Source/JavaScriptCore/runtime/JSString.h:532 >> + static_cast<JSRopeString*>(s)->resolveRope(exec); > > Can we move this branch into the callee? > > It feels unsafe that the callee assumes it will not be passed a non-substring rope, when nothing in its type signature guarantees it. I agree that would be better.
Created attachment 254629 [details] Patch Take 2, rope resolution moved to callee as requested by reviewers.
Comment on attachment 254629 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=254629&action=review r=me > Source/JavaScriptCore/runtime/JSString.h:314 > + // For now, let's not allow substrings with a rope base. > + // Resolve non-substring rope bases so we don't have to deal with it. Might be nice to elaborate on what we're trying to avoid. > Source/JavaScriptCore/runtime/JSString.h:315 > + if (base.isRope() && !base.isSubstring()) Is it possible for our base to be a substring? I think it is not, since all substrings will do the base forwarding above upon creation.
Landed in r185486