Open Bug 928014 Opened 8 years ago Updated 6 years ago

Investigate removing thumbnail IO worker thread

Categories

(Firefox :: General, defect)

defect
Not set
normal

Tracking

()

People

(Reporter: markh, Unassigned)

Details

(Whiteboard: p=0)

It should be possible to use OS.File's "builtin" worker thread for all IO performed by the thumbnail service.  I think :)
Whiteboard: [triage]
Whiteboard: [triage]
Whiteboard: p=0
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.
That sounds like a problem worth solving soon!
No longer blocks: fxdesktopbacklog
Flags: firefox-backlog+
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.
(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.
(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.
Flags: firefox-backlog+
Keywords: perf
(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?
(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.
You need to log in before you can comment on or make changes to this bug.