Bugzilla

Reporter

Comment 3

•

11 years ago

So we understand more about the severity problem, I'm currently having to add setTimeout/executeSoon into various places in our code to avoid InternalErrors caused by stack exhaustion.

This smells like a recipe for extremely fragile code to me.

Gregory Szorc [:gps]

Comment 4

•

11 years ago

See bug 845842 where promises and tasks might be causing FHR to exhaust stack space.

Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)

Reporter

Comment 5

•

11 years ago

I'm not sure this is 100% correct, but I think we could replace this code in the promise implementation:

      resolve = resolve ? attempt(resolve) : resolution
      function resolved(value) { deferred.resolve(resolve(value)) }

With the rather less elegant:

      var resolved;
      if (resolve) {
        var cresolve = resolve;
        resolved = function(value) {
          try {
            deferred.resolve(cresolve(value));
          }
          catch (error) {
            deferred.resolve({
              then: function(resolve, reject) {
                reject(error);
              }
            });
          }
        }
      }
      else {
        resolved = function(value) {
          deferred.resolve({
            then: function(resolve, reject) {
              resolve(value);
            }
          });
        }
      }

And it would cut the stack depth caused by promise.js down by 30%. Or in my latest crasher by 43 frames (crash at 204).

Further reduction may then be possible by inlining deferred.resolve, which would probably gain a further 30%.

So 60% reduction in stack depth seems possible to me.

Comment 6

•

11 years ago

Irakli said he'd like to tackle this in the work week.

Flags: needinfo?(rFobic)

Priority: -- → P2

Assignee

Comment 7

•

11 years ago

Joe, maybe we can do it during work week ?

Flags: needinfo?(rFobic)

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

Reporter

Comment 8

•

11 years ago

(In reply to Irakli Gozilalishvili [:irakli] [:gozala] [@gozala] from comment #7)
> Joe, maybe we can do it during work week ?

I'd like to think it has a higher priority that that! :(

I don't mind^H^H would like to discuss promises during the work week, but this is an important bug fix that doesn't require discussion.

Given the partial hack above, it seems easy to me???

Comment 9

•

11 years ago

I believe that we need both to optimize stack use and to work on a setTimeout-style implementation. To avoid needlessly complicating promise/core.js, we can certainly add a promise/async.js with the same API but a guarantee that setTimeout or nsITimer is always used.

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

Comment 10

•

11 years ago

Ah, it seems that we don't have promise/core.js anymore but a core/promise.js instead. It also seems that this file has regressed to an old version. Irakli, is that an accident? We spent considerable time improving the old version, it would be a shame to lose all that work.

Flags: needinfo?(rFobic)

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

Comment 11

•

11 years ago

(In reply to David Rajchenbach Teller [:Yoric] from comment #10)
> Ah, it seems that we don't have promise/core.js anymore but a
> core/promise.js instead. It also seems that this file has regressed to an
> old version. Irakli, is that an accident? We spent considerable time
> improving the old version, it would be a shame to lose all that work.

promise.js is maintained in the add-on SDK repo at github, any changes should be made there. When I did the uplift I looked and it didn't seem as if any changes were in promise/core.js that weren't already in the SDK repo, what is missing?

Comment 12

•

11 years ago

Many function names had changed. Most of the documentation had been rewritten. Additionally, the version of promise/core.js had been simplified with respect to promise.js.

Flags: needinfo?(rFobic)

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

Comment 13

•

11 years ago

(In reply to David Rajchenbach Teller [:Yoric] from comment #12)
> Many function names had changed. Most of the documentation had been
> rewritten. Additionally, the version of promise/core.js had been simplified
> with respect to promise.js.

Very strange, hg isn't showing any record of these changes. Can you point me to the bugs that landed them and I'll get them into the SDK repo.

Comment 14

•

11 years ago

Compare
http://mxr.mozilla.org/mozilla-beta/source/toolkit/addon-sdk/promise/core.js
and
http://mxr.mozilla.org/mozilla-central/source/addon-sdk/source/lib/sdk/core/promise.js

I believe that the changes landed as part of bug 708984.

Comment 15

•

11 years ago

(In reply to David Rajchenbach Teller [:Yoric] from comment #14)
> Compare
> http://mxr.mozilla.org/mozilla-beta/source/toolkit/addon-sdk/promise/core.js
> and
> http://mxr.mozilla.org/mozilla-central/source/addon-sdk/source/lib/sdk/core/
> promise.js
> 
> I believe that the changes landed as part of bug 708984.

Ugh sorry my bad, I thought all of that had been added to the SDK repo already, I'll get it updated tomorrow then we'll do an uplift to m-c on Tuesday

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

Reporter

Comment 16

•

11 years ago

In bug 845842 comment 36, I said:
> I think there are a number of reasons for including a setTimeout or similar in
> resolve/reject before the 'then' actions are called and a number against:
> ...

In bug 845842 comment 38, Gavin said:
> Given the summary of the arguments in comment 36 and the other context in this
> bug, I think we should switch our promises to be always-async. But I guess we
> should continue that particulary discussion in bug 842347.

We can't easily switch to always-async. I would support a change to default-async with optional sync, but even then we're going to need to do some careful checking that we're not breaking anything.

> To address this bug in the near term, it seems like we should just work around
> the Promise limitations in FHR code to the extent that that's possible (by
> avoiding them, or adjusting use of them).

There are some fairly trivial changes that we can make to promise (see comment 5 for an example of the type of thing I'm thinking of) that should give us significantly more stackroom.

Developer tools are going to need something like this, it seems like we should be able to concentrate our bodges in one place.

> It seems unlikely to me that JS engine changes (either an increased stack limit
> or stack use optimization) are required to address this, even in the longer term.

204 stack frames does seem like a very low limit to me. jimb/gps noted in bug 845842 comment 32 that this was down to a 37k cost per frame (worst case). Is it that simple? i.e. 7Mb stack isn't reasonable?

Comment 17

•

11 years ago

I will try and provide a setTimeout() version within the next few days, after Mossop has resolved the regression above.

:Gavin Sharp [email: gavin@gavinsharp.com]

Reporter

Comment 18

•

11 years ago

(In reply to David Rajchenbach Teller [:Yoric] from comment #17)
> I will try and provide a setTimeout() version within the next few days,
> after Mossop has resolved the regression above.

If you just throw a setTimeout into the resolve logic, then tests will fail. It's not that simple.

Gregory Szorc [:gps]

Comment 19

•

11 years ago

(In reply to Joe Walker [:jwalker] from comment #16)
> 204 stack frames does seem like a very low limit to me. jimb/gps noted in
> bug 845842 comment 32 that this was down to a 37k cost per frame (worst
> case). Is it that simple? i.e. 7Mb stack isn't reasonable?

The 37k per frame is a js::Interpret call in C++. This only happens for certain JS operations. Most normal JS function calls don't incur a new C++ frame and the stack growth in JS land is significantly less.

(In reply to David Rajchenbach Teller [:Yoric] from comment #17)
> I will try and provide a setTimeout() version within the next few days,
> after Mossop has resolved the regression above.

I would use the code at https://hg.mozilla.org/mozilla-central/file/default/services/common/utils.js#l93 instead of setTimeout() for scheduling something on a future tick. Maybe setTimeout() already does this if timeout == 0? Scheduling a full-blown timer that fires immediately seems wasteful when you can just directly push a callback onto the event loop queue. But, I don't know much about how all this works. I could be wrong.

Comment 20

•

11 years ago

(In reply to Joe Walker [:jwalker] from comment #18)
> (In reply to David Rajchenbach Teller [:Yoric] from comment #17)
> > I will try and provide a setTimeout() version within the next few days,
> > after Mossop has resolved the regression above.
> 
> If you just throw a setTimeout into the resolve logic, then tests will fail.
> It's not that simple.

Can't we adjust the tests? Are there so many that that becomes really painful or something?

Reporter

Comment 21

•

11 years ago

(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #20)
> (In reply to Joe Walker [:jwalker] from comment #18)
> > (In reply to David Rajchenbach Teller [:Yoric] from comment #17)
> > > I will try and provide a setTimeout() version within the next few days,
> > > after Mossop has resolved the regression above.
> > 
> > If you just throw a setTimeout into the resolve logic, then tests will fail.
> > It's not that simple.
> 
> Can't we adjust the tests? Are there so many that that becomes really
> painful or something?

It's down to the 2nd of my reasons to consider synchronous promises as a valid option:

> Async code is viral, in contexts with some degree of polymorphism it can be a
> handy shortcut to reason. "I know this promise can only happen synchronously,
> so I can save the complexity and use the value without waiting" - i.e. preventing
> the unnecessary spread of asynchronisity

Bug 685526 makes command line parse parameters asynchronously. It's important for a remotable command-line to make our tools work well with b2g.

Commands are (to the command line) a special type of parameter, but we can *always* parse them synchronously, so in a number of places we can save ourselves lots of complexity by reasoning "We know parameter is a command and so parsing it is synchronous, so lets get the value now rather than taking on-board a chunk of complexity that doesn't achieve anything"

Reporter

Comment 22

•

11 years ago

Irakli - I think this is a serious issue, what do you think we should do?

Flags: needinfo?(rFobic)

Assignee

Updated

•

11 years ago

Assignee: nobody → rFobic

Assignee

Comment 23

•

11 years ago

I'll be trying to integrate Joe's suggestion and submitting a pull request I'll ask Joe for feedback to make sure it solves the problem as I don't really know how to put a test that would check for stack length.

Yoric Can we please make another tire on top of this for async stuff ? Meaning can we have another module that wraps this one and delays resolutions so this won't be concerned with timers / or pushing callbacks to the thread.

Although I personally would prefer `delay(promise, ms)` instead, but maybe it's too much if that's what you want all the time.

Flags: needinfo?(rFobic)

Assignee

Comment 24

•

11 years ago

(In reply to David Rajchenbach Teller [:Yoric] from comment #10)
> Ah, it seems that we don't have promise/core.js anymore but a
> core/promise.js instead. It also seems that this file has regressed to an
> old version. Irakli, is that an accident? We spent considerable time
> improving the old version, it would be a shame to lose all that work.

I think location change happened by accident when we were landing SDK to central. I'm open to changing it's location back to where it used to be or to wherever we think makes more sense.

Assignee

Comment 25

•

11 years ago

Attached file Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/844 — Details

Pointer to Github pull-request

Assignee

Updated

•

11 years ago

Attachment #722522 - Flags: review?(dtownsend+bugmail)

Attachment #722522 - Flags: feedback?(jwalker)

Panos Astithas (he/him) [:past] (please ni?)

Updated

•

11 years ago

Attachment #722522 - Flags: review?(dtownsend+bugmail) → review+

Comment 26

•

11 years ago

(In reply to Irakli Gozilalishvili [:irakli] [:gozala] [@gozala] from comment #23)
> I'll be trying to integrate Joe's suggestion and submitting a pull request
> I'll ask Joe for feedback to make sure it solves the problem as I don't
> really know how to put a test that would check for stack length.

Maybe putting some code in place to slightly exceed the 204 stack frame limit that Joe mentioned and then catching an InternalError?

Alexandre Poirot [:ochameau]

Comment 27

•

11 years ago

https://developer.mozilla.org/en-US/docs/Components.stack ?
stack after - stack before < reasonable stack count

Reporter

Updated

•

11 years ago

Attachment #722522 - Flags: feedback?(jwalker) → feedback+

Reporter

Comment 28

•

11 years ago

(In reply to Alexandre Poirot (:ochameau) from comment #27)
> https://developer.mozilla.org/en-US/docs/Components.stack ?
> stack after - stack before < reasonable stack count

The problem here is that we're blowing the C++ stack not the JS stack. The trick is knowing what JS actions cause a call into C++ code and back into JS. Clearly native calls are C++, as are most DOM operations, getters/setters, generators, etc.

So not all stack frames are equal some have a C++ stack weight of 0k, but others have a weight of 37k.

[github robot]

Comment 29

•

11 years ago

Commit pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/0d83daa117c3bef7f4a3069c40de9fc207b3c7d1
Merge pull request #844 from Gozala/bug/promise-stack@842347

Bug 842347 - Inline utility functions to reduce a promise stack size. r=@Mossop

Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)

Assignee

Comment 30

•

11 years ago

Joe could you please let me know weather this solved the InternalError problem and if it did I'd like to close this bug. That does not mean that we should stop working on reducing stack further, but severity of the issue can be decreased.

Updated

•

11 years ago

Flags: needinfo?(jwalker)

[github robot]

Comment 31

•

11 years ago

Commit pushed to integration at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/0d83daa117c3bef7f4a3069c40de9fc207b3c7d1
Merge pull request #844 from Gozala/bug/promise-stack@842347