928014 - Investigate removing thumbnail IO worker thread

Status: NEW

(Firefox :: General, defect)

Description

[Mark Hammond [:markh] [:mhammond]]

It should be possible to use OS.File's "builtin" worker thread for all IO performed by the thumbnail service.  I think :)

Comment 1

[Mark Hammond [:markh] [:mhammond]]

Not sure this bug solves the problem, but while chasing oranges, I recently noticed that the creation of a 22028 byte thumbnail causes a JSON payload of 244375 bytes to be sent to PageThumbsWorker.jsm - the vast majority is:

{"0":137,"1":80,"2":78,"3":71,"4":13,"5":10 ...

which is the png data.  Ideally we could avoid JSON and transfer a byte array.

Comment 2

[:Gavin Sharp [email: [e-mail]]]

That sounds like a problem worth solving soon!

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

wipe() might be doable now with plain OS.File APIs but expiring thumbnails without a custom worker sounds like a lot of communication overhead if we have to iterate the directory and then remove each file separately.

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Mark Hammond [:markh] from comment #1)
> Not sure this bug solves the problem, but while chasing oranges, I recently
> noticed that the creation of a 22028 byte thumbnail causes a JSON payload of
> 244375 bytes to be sent to PageThumbsWorker.jsm - the vast majority is:
> 
> {"0":137,"1":80,"2":78,"3":71,"4":13,"5":10 ...

Are you sure this isn't just a weird representation of an actual array? PageThumbsStorage.writeData() sends a Uint8Array so that should be fine.

Comment 5

[Mark Hammond [:markh] [:mhammond]]

(In reply to Tim Taubert [:ttaubert] from comment #4)

> Are you sure this isn't just a weird representation of an actual array?
> PageThumbsStorage.writeData() sends a Uint8Array so that should be fine.

sigh - no, you are correct - re-looking at this, I can't see any evidence of it being true.  That's what I get for playing with this at night after beer-o'clock.

Sorry for the noise.

Comment 6

[David Teller [:Yoric] - still alive but not very active]

(In reply to Tim Taubert [:ttaubert] from comment #4)
> > {"0":137,"1":80,"2":78,"3":71,"4":13,"5":10 ...
> 
> Are you sure this isn't just a weird representation of an actual array?
> PageThumbsStorage.writeData() sends a Uint8Array so that should be fine.

Yeah, that's what happens when you attempt to stringify a Uint8Array.
Now, looking at the source code, I have the impression that we are copying the array instead of transferring it, which is not good.

> wipe() might be doable now with plain OS.File APIs but expiring thumbnails without a custom worker sounds like a lot of communication overhead if we have to iterate the directory and then remove each file separately.

I realize that it's suboptimal, but how common is that?

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

(In reply to David Rajchenbach Teller [:Yoric] from comment #6)
> > Are you sure this isn't just a weird representation of an actual array?
> > PageThumbsStorage.writeData() sends a Uint8Array so that should be fine.
> 
> Yeah, that's what happens when you attempt to stringify a Uint8Array.
> Now, looking at the source code, I have the impression that we are copying
> the array instead of transferring it, which is not good.

TaskUtils.readBlob() returns an ArrayBuffer that then is passed to the Uint8Array constructor. According to MDN [1] passing an ArrayBuffer just returns a view and uses the given buffer as its storage.

[1] https://developer.mozilla.org/en-US/docs/Web/API/Uint8Array

> > wipe() might be doable now with plain OS.File APIs but expiring thumbnails without a custom worker sounds like a lot of communication overhead if we have to iterate the directory and then remove each file separately.
> 
> I realize that it's suboptimal, but how common is that?

We expire thumbnails approximately once per hour. The overhead then depends on how many thumbnails you gathered since the last expiration.