Closed Bug 649889 Opened 13 years ago Closed 7 years ago

[Tracking] Avoid, discourage and deprecate synchronous disk I/O

Categories

(Add-on SDK Graveyard :: General, defect, P4)

defect

Tracking

(tracking-b2g:backlog)

RESOLVED INCOMPLETE
tracking-b2g backlog

People

(Reporter: philikon, Unassigned)

References

(Depends on 1 open bug)

Details

As with lots of other Gecko APIs, the SDK creates nicely wraps Gecko's file APIs. I'm sad to see, however, that apart from TextWriter.writeAsync, support for asynchronous I/O is nowhere to be found. Even worse, higher level modules like simple-storage.js build on top file.js and do all their work synchronously.

Synchronous disk I/O is a pretty big footgun for causing browser slow down. I don't think we should encourage third party developers to do sync I/O. Ever. I would suggest to add support for async I/O everywhere and to deprecate synchronous I/O methods. Of course this will affect both low level modules as well as high level modules.

Of course, e10s might mitigate some problems caused by naively written, synchronous code. However, e10s still is a many many months away. Plus, any sync I/O would still lock up the JetPack process. Bottom line, I don't think e10s would be the silver bullet for this problem at all.
P.S.: I apologize in advance to sdwilsh for not using an Oxford Comma in the bug summary. http://youtu.be/P_i1xk07o4g
Priority: -- → P1
Target Milestone: --- → Future
Just a bit of background on simple-storage.  Originally I proposed deferring the evaluation of an add-on's main.js until its simple storage had been asynchronously loaded.  It's only the initial load that's sync.  (We could defer main.js evaluation until each API that requires initial loading of resources had asyncly loaded its resources, if we were to add other such APIs in the future.)

Atul pointed out that some add-ons really do need to run at startup though, or when the user's pages are loaded by session restore, or otherwise before such a deferment.  I didn't think of it at the time, but I could have made a counter argument that all early events could just be queued up and then sent to the add-on when it was time to load it, although that's not quite the same thing.

What I did propose was that an event could be fired when simple-storage had finished async loading, and in order to use simple-storage an add-on would have to listen for the event and respond to it.  But that's not very friendly, so I didn't press it.

simple-storage in the Jetpack Prototype was originally async, so using it looked like this:

  jetpack.simpleStorage.get("foo", function () {
    console.log("foo gotten");
    jetpack.simpleStorage.set("bar", function () {
      console.log("bar successfully set");
    });
  });

But that gets awful pretty quick, and it bumps up against the Jetpack goal of being nice to use.

Above all else, simple-storage was intended to be simple, and everyone agreed that the current API made a reasonable trade-off of sync I/O for the desired simplicity.  Certainly though, if there were a way to preserve simplicity and do the initial I/O async, that would be best.
(In reply to comment #2)
> Atul pointed out that some add-ons really do need to run at startup though,

As in "but I really really do want to slow your browser startup down"?!? Seems like a perfect footgun to me.

> or when
> the user's pages are loaded by session restore, or otherwise before such a
> deferment.

I was sad to see that bootstrappable addons already get called way too early. I was hoping JetPack wouldn't fire up until the browser has actually finished loading. This is why the terrorists^H^H^H^H^H^H^H^H^H^H Chrome guys win! But seriously, we allow our add-ons to do way too much stuff. If you ask me, add-ons like BarTab shouldn't even be possible.

Anyway, given the route we're going with e10s, I don't think add-ons will be even be able to make use of being called so early on because all access back to the chrome window will be async anyway, so there's no guarantee that they can influence the startup path in time.

> I didn't think of it at the time, but I could have made a counter
> argument that all early events could just be queued up and then sent to the
> add-on when it was time to load it, although that's not quite the same thing.

Seems like a better idea, though.

> What I did propose was that an event could be fired when simple-storage had
> finished async loading, and in order to use simple-storage an add-on would have
> to listen for the event and respond to it.  But that's not very friendly, so I
> didn't press it.

How's that not friendly? Seems perfectly fine.

> simple-storage in the Jetpack Prototype was originally async, so using it
> looked like this:
> 
>   jetpack.simpleStorage.get("foo", function () {
>     console.log("foo gotten");
>     jetpack.simpleStorage.set("bar", function () {
>       console.log("bar successfully set");
>     });
>   });
> 
> But that gets awful pretty quick, and it bumps up against the Jetpack goal of
> being nice to use.

How is that not nice to use? Isn't JetPack aimed at the average Joe Sixpack Web Developer? They would be very familiar with this sort of async stuff. jQuery, dojo, YUI, require.js all have plenty of async APIs. Heck, most of them do async *module loading* which we don't even do!

> Above all else, simple-storage was intended to be simple, and everyone agreed
> that the current API made a reasonable trade-off of sync I/O for the desired
> simplicity.  Certainly though, if there were a way to preserve simplicity and
> do the initial I/O async, that would be best.

I think this is a false dichotomy. Async does not mean it's hard. Async also does not mean it's unpopular. Definitely not with the web developer crowd. No need to be condescending to developers here. If anything, we should set a good example.

That said, I agree that callback can be syntactically tiring. There are good patterns that make it less so, but I give you that. Fortunately, SpiderMonkey has generators and other goodies that let you write async code that looks very much like sync code. Dave Herman has a great library built around that: https://github.com/dherman/taskjs
You can always load the add-on's simple-storage data into memory when it loads the module if you want calls to it to by sync.  Then you don't do I/O and still get your sync API (at the cost of memory usage, but that helps to enforce it to be used for "simple" things).

FWIW, this being classified as future is saddening.  Add-ons can very trivially undo all the hard work we are doing on the platform to reduce I/O on the GUI thread.
(In reply to comment #4)
> FWIW, this being classified as future is saddening.  Add-ons can very trivially
> undo all the hard work we are doing on the platform to reduce I/O on the GUI
> thread.

Shawn: understood.  However, this is just one of many things the core Jetpack team would like to do that we aren't going to get done by the time we ship Add-on SDK 1.0 in June.  In the grand scheme of things, we're much better off shipping all the great work we've done than holding the release until we do everything else we want to do, including additional improvements like this to the kinds of horkage addons can inflict.

Also, bear in mind that "future" means something different for us than it does for Firefox/platform, where it appears to mean "this will probably never happen, but we're not sure enough yet to close it".  For the Jetpack project, "future" means only "not 1.0", because we haven't targeted any work past 1.0 at this point.

Once we have 1.0 in hand and a plan for future releases, we're going to re-triage bugs and decide where they fit.  And we've marked this P1 to indicate that we think it's of the highest priority, which means we're likely to come back to it sooner than later.

(It goes without saying, but of course we're also happy to accept contributions from outside the core team for the 1.0 release.)
(In reply to comment #3)
> loading. This is why the terrorists^H^H^H^H^H^H^H^H^H^H Chrome guys win! But
> seriously, we allow our add-ons to do way too much stuff. If you ask me,
> add-ons like BarTab shouldn't even be possible.

It's too late for us, the genie's out of the bottle.  If Jetpack doesn't provide an API for something (or if its API sucks), you grab Components.classes or write a traditional extension.  If you're a seasoned add-on developer, you probably ignore Jetpack in the first place.  Chrome's not dragging around a seven-year-old legacy.

But this is kind of off topic.

> How is that not nice to use? Isn't JetPack aimed at the average Joe Sixpack Web
> Developer? They would be very familiar with this sort of async stuff.

That is not what our experience with many of our users has borne out unfortunately.  But maybe we just have bad async APIs.

> I think this is a false dichotomy. Async does not mean it's hard. Async also
> does not mean it's unpopular. Definitely not with the web developer crowd.

I invite you to make this argument in the Jetpack user group.  Here it is:

http://groups.google.com/group/mozilla-labs-jetpack

BTW, you sound like me circa late 2009, which is to say, I made all of your arguments when Jetpack was getting started and lost.  It turned out there were some arguments I'm glad I lost, because no one will use your non-"footgun" API if it sucks.

(In reply to comment #4)
> You can always load the add-on's simple-storage data into memory when it loads
> the module if you want calls to it to by sync.  Then you don't do I/O and still
> get your sync API

Load the add-on's simple-storage data into memory -- but from where?

Currently an add-on's simple-storage data is loaded into memory when the add-on first references require("simple-storage").storage -- but of course it's loaded from disk, syncly.
Individual bugs should be filed for each instance of synchronous code.
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → INVALID
Could somebody commit to doing that?  I don't have time, but if we continue on the path we're on, we are going to end up shipping the SDK with synchronous disk I/O abound.
(In reply to Shawn Wilsher :sdwilsh from comment #8)
> Could somebody commit to doing that?

I'd be happy if the Jetpack folks would like some help.

> I don't have time, but if we continue
> on the path we're on, we are going to end up shipping the SDK with
> synchronous disk I/O abound.

I agree.

Should we perhaps not keep this bug open and use it as a tracking bug (which also means it should *depend* on other bugs, not block them).
(In reply to Philipp von Weitershausen [:philikon] from comment #9)
> I'd be happy if the Jetpack folks would like some help.

Have at it. :)


> Should we perhaps not keep this bug open and use it as a tracking bug

Reopening and flagging the bug for myk, mossop and dcm's weekly triage meeting. Do we want a tracking bug? Or would this warrant a feature page?

Philikon, if you file the bugs before they get to this, just set them all as blocking this one, and I can move them around if they don't want this tracking bug.

> which also means it should *depend* on other bugs, not block them).

Yeah. Flipping bug 682631 around.
No longer blocks: 682631
Status: RESOLVED → REOPENED
Depends on: 682631
Resolution: INVALID → ---
Whiteboard: [triage:followup]
Personally, I think this abstract vision for the SDK is better as a feature page than a tracking bug, as are other such abstract visions for which we've created feature pages, like Hug the Web Harder <https://wiki.mozilla.org/Features/Jetpack/Make_Add-on_SDK_Hug_the_Web_Harder> and Simplify the Add-on SDK <https://wiki.mozilla.org/Features/Jetpack/Simplify_the_Add-on_SDK>.
(Pushing all open bugs to the --- milestone for the new triage system)
Target Milestone: Future → ---
Removing the triage flag from the bug.

Can someone make a feature page for this?
Whiteboard: [triage:followup]
Note: bug 563742 introduces a new API that (among other things) warns when used synchronously on the main thread.
(In reply to David Rajchenbach Teller [:Yoric] from comment #14)
> Note: bug 563742 introduces a new API that (among other things) warns when
> used synchronously on the main thread.
Nobody reads warnings, so adding another footgun API means it's just going to get used...
There is also an option to turn all such warnings into exceptions.
(In reply to David Rajchenbach Teller [:Yoric] from comment #16)
> There is also an option to turn all such warnings into exceptions.

... which no one will enable.
Ok, so what do you suggest?

If we provide an API that is too strict, people will not use it. If we provide an API that is too lenient, people will ignore warnings. I chose what I hope is the most pragmatic choice with a lenient API and a (global) flag that can set it to strict.
(In reply to David Rajchenbach Teller [:Yoric] from comment #18)
> Ok, so what do you suggest?

I think Shawn and I have time and again made a pretty clear case for providing asynchronous, off-thread I/O APIs only.

> If we provide an API that is too strict, people will not use it.

How do you arrive at that conclusion? If it's the *only* API we provide, people will use it. I think the whole "async is hard and unpopular, people won't use it" argument is kind of bogus. See the last two paragraphs of my comment 3, for instance.

> If we provide an API that is too lenient, people will ignore warnings. I chose
> what I hope is the most pragmatic choice with a lenient API and a (global)
> flag that can set it to strict.

That's not a pragmatic choice, it's a cop out. What you're doing there is creating ways for people fabricate footguns and make Firefox less snappy, and then telling them it's their own fault if they use it. We gotta stop making those footguns. Especially with electrolysis off the table for desktop Firefox, there's very little margin for inefficiency.
(In reply to Philipp von Weitershausen [:philikon] from comment #19)

> > If we provide an API that is too strict, people will not use it.

Callback-oriented programming is pretty easy in JS. Every web developer who's used a mainstream framework or XHR knows how it works. And synchronous I/O is simply unjustifiable: the trivial improvement in simplicity is completely demolished by the enormous performance implications.

The only way to get sane behavior from developers is to make *all* exposed APIs sane, safe, and performant.

Don't ship footguns, and people won't end up with holes in their feet.
And to add one more voice to the choir, if we rely on reviews and other best practices to avoid sync main thread i/o the code we ship with Firefox might end up doing the right thing (though based on past experience I doubt it) but addons won't.
Ok, ok, I surrender :)

Here's what I'll do: leave the ability to switch the exception on/off for unit testing, but set it "on" by default.
No longer depends on: 801610
Are there any plans to address this in the SDK?

Do we have any idea what kind of compatibility constraints there are for APIs implemented in file.js?
Blocks: 572459
No longer depends on: 975996
No longer depends on: 975750
No longer depends on: OMTPlaces
No longer depends on: 975988
No longer blocks: 572459
This needs another look over.
Flags: needinfo?
Priority: P1 → --
This is really a meta bug. We should be filing bugs to remove synchronous disk I/O from our modules and putting a deprecated warning in the io/file module.
Flags: needinfo?
Priority: -- → P4
Summary: Avoid, discourage and deprecate synchronous disk I/O → [Tracking] Avoid, discourage and deprecate synchronous disk I/O
Depends on: 1039490
[Tracking Requested - why for this release]:
https://bugzilla.mozilla.org/show_bug.cgi?id=1399562
Status: REOPENED → RESOLVED
Closed: 13 years ago7 years ago
Resolution: --- → INCOMPLETE
You need to log in before you can comment on or make changes to this bug.