Bug 208683 - Delete encoded data from CachedImage once image is created
Summary: Delete encoded data from CachedImage once image is created
Status: RESOLVED INVALID
Alias: None
Product: WebKit
Classification: Unclassified
Component: Images (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Basuke Suzuki
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-03-05 18:32 PST by Basuke Suzuki
Modified: 2020-05-11 16:10 PDT (History)
2 users (show)

See Also:


Attachments
PATCH (1.24 KB, patch)
2020-03-05 20:42 PST, Basuke Suzuki
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Basuke Suzuki 2020-03-05 18:32:54 PST
Encoded data won't be used after image is successfully created. It can be cleared.
Comment 1 Basuke Suzuki 2020-03-05 20:42:05 PST
Created attachment 392667 [details]
PATCH
Comment 2 Basuke Suzuki 2020-03-09 11:57:40 PDT
Copied from Slack conversation: https://webkit.slack.com/archives/CU64U6FDW/p1583469841216300

smfr
I’m suspicious of this. doesn’t memory pressure clear the image cache, which means we’ll need to go back and decoded using the encoded data?

yusukesuzuki  4 days ago
Yes. Memory pressure clears the decoded data. So, we can clear the data only if (1) the decoded content can represent the entire value, and (2) the memory pressure handler does not clear it.

Let’s consider about the example,

Unicode source CachedScript data and it is not file-backed.

This case, we are doubling the size basically because unicode string cannot be represented from SharedBuffer without decoding, and SharedBuffer needs to keep the content while it is not file backed.

In this case, this is reasonable that, purging SharedBuffer and holding decoded data, and do not purge decoded data when the low memory handler happens

(1) is it possible that we see many non-file backed data? (edited) 

A1. old Membuster is saying yes. We should measure this on new Membuster. (edited) 

A2. On iOS, yes. Our threshold for file backed data is very high (16KB) in iOS. (edited) 

basuke:beer:
Ah, that make sense. Unfortunately our platform do not handle low memory situation well. That’s why I didn’t hit the case. 

I’ll revisit this soon after taking care of memory pressure.

yusukesuzuki
Right, OK. But at the same time, I believe that we should not rely on low memory handler to reduce memory usage. In the ideal world, we should have control system which attempt to shrink memory well when we are doing idle. And I (personally) believe that JSC GC could take this role (managing all the memory usage). @smfr Do you have some idea? (edited) 

smfr
i’m not sure GC should be this thing. I’d prefer some kind of cache manger that knows about all the caches

yusukesuzuki
Why I would like to put this in GC is that I believe that we should attempt to make memory small even if we are using JSC framework. But it is also good that some manager can make memory shrink if this manager works well in JSC framework too.
Comment 3 Basuke Suzuki 2020-05-11 16:10:57 PDT
Encoded image shouldn't be deleted as Simon says.