Opened 5 years ago

Closed 5 years ago

Last modified 4 years ago

#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@…, 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).

Change History (8)

comment:1 Changed 5 years ago by mapx

  • Cc smultron45@… added

comment:2 Changed 5 years ago by philll

  • Cc trev added

@trev: you might want to look into this.

comment:3 Changed 5 years ago 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 5 years ago 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 5 years ago 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 5 years ago 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).

Last edited 5 years ago by trev (previous) (diff)

comment:7 Changed 5 years ago 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 4 years ago by philll

  • Platform changed from Firefox/Firefox Mobile to Firefox

Made Firefox and Firefox mobile available as seperate platforms.

Note: See TracTickets for help on using tickets.