Bug 196878 - Bytecode cache should not encode the SourceProvider for UnlinkedFunctionExecutable's classSource
Summary: Bytecode cache should not encode the SourceProvider for UnlinkedFunctionExecu...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Tadeu Zagallo
URL:
Keywords: InRadar
Depends on:
Blocks: 196881
  Show dependency treegraph
 
Reported: 2019-04-12 14:21 PDT by Tadeu Zagallo
Modified: 2019-04-15 15:24 PDT (History)
8 users (show)

See Also:


Attachments
Patch (8.05 KB, patch)
2019-04-12 14:39 PDT, Tadeu Zagallo
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tadeu Zagallo 2019-04-12 14:21:49 PDT
...
Comment 1 Tadeu Zagallo 2019-04-12 14:39:19 PDT
Created attachment 367346 [details]
Patch
Comment 2 Saam Barati 2019-04-15 14:27:37 PDT
Comment on attachment 367346 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:14
> +        incremental cache, each UnlinkedFunctionCodeBlock is encoded in isolation, which
> +        means we can no longer deduplicate it and the full program text was being encoded
> +        multiple times in the cache.

How is this just not a general problem that affects all types that were previously being de-duplicated during encoding?
Comment 3 Filip Pizlo 2019-04-15 14:30:39 PDT
Comment on attachment 367346 [details]
Patch

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

>> Source/JavaScriptCore/ChangeLog:14
>> +        multiple times in the cache.
> 
> How is this just not a general problem that affects all types that were previously being de-duplicated during encoding?

I'm also curious.
Comment 4 Saam Barati 2019-04-15 14:30:40 PDT
Comment on attachment 367346 [details]
Patch

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

r=me

>> Source/JavaScriptCore/ChangeLog:14
>> +        multiple times in the cache.
> 
> How is this just not a general problem that affects all types that were previously being de-duplicated during encoding?

can we open a bug so we can handle the general case?
Comment 5 Saam Barati 2019-04-15 14:31:28 PDT
(In reply to Saam Barati from comment #4)
> Comment on attachment 367346 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=367346&action=review
> 
> r=me
> 
> >> Source/JavaScriptCore/ChangeLog:14
> >> +        multiple times in the cache.
> > 
> > How is this just not a general problem that affects all types that were previously being de-duplicated during encoding?
> 
> can we open a bug so we can handle the general case?

Or maybe it's worth just solving instead of solving this one specific case we know is especially bad.
Comment 6 Tadeu Zagallo 2019-04-15 15:07:54 PDT
Comment on attachment 367346 [details]
Patch

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

>>>>> Source/JavaScriptCore/ChangeLog:14
>>>>> +        multiple times in the cache.
>>>> 
>>>> How is this just not a general problem that affects all types that were previously being de-duplicated during encoding?
>>> 
>>> I'm also curious.
>> 
>> can we open a bug so we can handle the general case?
> 
> Or maybe it's worth just solving instead of solving this one specific case we know is especially bad.

This is a general problem, but it's a bit more complicated to solve in the general case. As we discussed on IRC, the encoder map might end up with dead pointers, but maybe we can batch the updates to minimize the duplications. In which case, I still think that this case is bad enough that we *never* want it to be duplicated. Plus, I remember we've discussed in the past that it could be useful to be able to inject a SourceProvider during decoding, instead of allocating a new one. I'll land this as is, so that I can start running JS2 as a bytecode cache test and file another bug fixing the general case.
Comment 7 WebKit Commit Bot 2019-04-15 15:23:44 PDT
Comment on attachment 367346 [details]
Patch

Clearing flags on attachment: 367346

Committed r244300: <https://trac.webkit.org/changeset/244300>
Comment 8 WebKit Commit Bot 2019-04-15 15:23:46 PDT
All reviewed patches have been landed.  Closing bug.
Comment 9 Radar WebKit Bug Importer 2019-04-15 15:24:23 PDT
<rdar://problem/49920240>