Opened on 05/13/2014 at 08:34:37 PM
Closed on 07/15/2014 at 01:08:40 PM
Last modified on 05/20/2015 at 02:22:39 PM
#476 closed change (rejected)
Need improvements to OS.File.writeAtomic()?
| Reported by: | David Rajchenbach-Teller | Assignee: | |
|---|---|---|---|
| Priority: | Unknown | Milestone: | |
| Module: | Adblock-Plus-for-Firefox | Keywords: | |
| Cc: | smultron45@gmail.com, trev | Blocked By: | |
| Blocking: | Platform: | Firefox | |
| Ready: | no | Confidential: | no |
| Tester: | Verified working: | no | |
| Review URL(s): | |||
Description
According to comments, IO.writeToFile mimics OS.File.writeAtomic but writes in chunks. If this is due to a performance issue with OS.File.writeAtomic, could you file a OS.File bug? I'll be happy to fix the issue directly in Gecko (or to mentor someone to work on it).
Attachments (0)
Change History (8)
comment:1 Changed on 05/14/2014 at 08:01:17 AM by mapx
- Cc smultron45@gmail.com added
comment:2 Changed on 05/15/2014 at 03:31:36 PM by philll
- Cc trev added
comment:3 Changed on 05/15/2014 at 06:37:58 PM by trev
The reason why I didn't use OS.File.writeAtomic was simply that I didn't want to generate all the data at once - and then one more copy of it as a typed buffer. IO.writeToFile gets a generator, the data will only be generated as needed and discarded immediately. So this is primarily about the memory footprint. Additionally, this way we don't block the main thread while the data is being generated but this isn't the main reason (time required was around 50ms).
comment:4 Changed on 06/04/2014 at 12:42:34 PM by David Rajchenbach-Teller
I have never heard of a 50ms lag caused by OS.File.writeAtomic. Or is it the generation itself that takes ~50ms?
comment:5 Changed on 06/04/2014 at 02:01:28 PM by trev
I was talking about the data generation, that takes around 50ms in total. Sure, other work-arounds are possible here.
comment:6 Changed on 07/15/2014 at 01:08:11 PM by trev
- Platform set to Unknown
To conclude here: while using OS.File.writeAtomic() would certainly be simpler, our current code isn't too complicated either and it is well-tested. In addition, OS.File.writeAtomic() currently isn't a good solution for writing large files. So I don't think we want to change anything right now, other than maybe tweaking buffer sizes (filed #988 on that).
comment:7 Changed on 07/15/2014 at 01:08:40 PM by trev
- Component changed from Unknown to Adblock-Plus-for-Firefox
- Platform changed from Unknown to Firefox/Firefox Mobile
- Resolution set to rejected
- Status changed from new to closed
comment:8 Changed on 05/20/2015 at 02:22:39 PM by philll
- Platform changed from Firefox/Firefox Mobile to Firefox
Made Firefox and Firefox mobile available as seperate platforms.
@trev: you might want to look into this.