Bug 196409 – Cache bytecode for jsc.cpp helpers and fix CachedStringImpl

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Tadeu Zagallo]

...

[Tadeu Zagallo]

Created attachment 366309 [details]
Patch

[Saam Barati]

Comment on attachment 366309 [details]
Patch

This needs a test.

[Saam Barati]

Comment on attachment 366309 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:18
> +        had different shapes. That was a problem if a StringImpl* was originally cached as a
> +        CachedPtr<CachedStringImpl>, and later if we tried to cache it as CachedPtr<CachedUniquedStringImpl>
> +        we would find a cache entry and store the offset of the already cached CachedStringImpl.
> +        We keep the same behavior, but make CachedStringImpl and CachedUniquedStringImpl share the same shape.

I don't understand your explanation here. What exactly were we storing that was the problem?

[Tadeu Zagallo]

Comment on attachment 366309 [details]
Patch

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

>> Source/JavaScriptCore/ChangeLog:18
>> +        We keep the same behavior, but make CachedStringImpl and CachedUniquedStringImpl share the same shape.
> 
> I don't understand your explanation here. What exactly were we storing that was the problem?

Yeah, this explanation got really confusing, I think I'll just list it as a sequence of steps. The problem is that the encoder keeps a map of pointers to offsets of already cached objects, in order to avoid caching the same object twice. If we have the same pointer being cached as two different cached types, that's an issue. I'll update this.

[Tadeu Zagallo]

Created attachment 366362 [details]
Patch

[Saam Barati]

Comment on attachment 366362 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:30
> +        which has a different shape and we crash.

Thanks for this explanation. It makes sense.

You should also write a sentence on what the fix is.

I wonder though if this shows us a more general bug in our encoding/decoding.

I wonder if we should have a more general solution of type matters when encoding. So even if you have the same pointer twice, then you should encode it twice? Maybe this leads to other issues. It probably does, like if you encode a CodeBlock twice based on its type, that's probably bad.

But the issue we have now is a base class needs to be aware of its subclasses, which ain't nice.

[Tadeu Zagallo]

Comment on attachment 366362 [details]
Patch

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

>> Source/JavaScriptCore/ChangeLog:30
>> +        which has a different shape and we crash.
> 
> Thanks for this explanation. It makes sense.
> 
> You should also write a sentence on what the fix is.
> 
> I wonder though if this shows us a more general bug in our encoding/decoding.
> 
> I wonder if we should have a more general solution of type matters when encoding. So even if you have the same pointer twice, then you should encode it twice? Maybe this leads to other issues. It probably does, like if you encode a CodeBlock twice based on its type, that's probably bad.
> 
> But the issue we have now is a base class needs to be aware of its subclasses, which ain't nice.

I'll add a line about the fix.

I think there are a couple things that should be fixed:
1) we can make UnlinkedStringJumpTable use UniquedStringImpl, this way we don't have to worry about this specific problem anymore, since that is the only place that uses CachedStringImpl.
2) You are right about the subclasses issue, but I think if we fix 1 there's no other place where we intentionally rely on that. e.g. I don't think we encode the same code block as both UnlinkedCodeBlock and as its subclass. In fact, we don't handle that properly, so I think it'd be interesting to add some validation.

I'll land this with the update to the ChangeLog and work on these 2 follow-ups.

[Tadeu Zagallo]

Created attachment 366712 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 366712 [details]
Patch for landing

Clearing flags on attachment: 366712

Committed r243869: <https://trac.webkit.org/changeset/243869>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/49608083>