Closed
Bug 842347
Opened 12 years ago
Closed 12 years ago
Reduce stack depth caused by our promise implementation
Categories
(Add-on SDK Graveyard :: General, defect, P2)
Add-on SDK Graveyard
General
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: jwalker, Assigned: irakli)
References
Details
Attachments
(1 file)
Bug 810490 called for "Much reduced stack depth"
> Debugging promises is made even harder by the large number of stack frames
> created by even trivial operations. Some of the other proposals may be possible
> using some type of a wrapper, but this one wouldn't be
Some trivial inlining could achieve this.
This problem is annoying, but I currently thinking that it might also be the cause of some bugs. One of my tests is currently giving an InternalError when I try to call any function, which I wonder might be a stack depth problem.
For reference 70% of the frames in this stack are due to promise.jsm. The remaining 30% are due to the template engine, command line, developer tools, test suite and Firefox.
Templater.prototype._processForEachMember/<@resource:///modules/devtools/Templater.jsm:340
Templater.prototype._handleAsync@resource:///modules/devtools/Templater.jsm:463
Templater.prototype._processForEachMember@resource:///modules/devtools/Templater.jsm:361
Templater.prototype._processForEachLoop/<@resource:///modules/devtools/Templater.jsm:308
Templater.prototype._processForEachLoop@resource:///modules/devtools/Templater.jsm:309
Templater.prototype._processForEach/<@resource:///modules/devtools/Templater.jsm:281
Templater.prototype._handleAsync/<@resource:///modules/devtools/Templater.jsm:454
effort@resource://gre/modules/commonjs/sdk/core/promise.js:57
resolved@resource://gre/modules/commonjs/sdk/core/promise.js:122
then@resource://gre/modules/commonjs/sdk/core/promise.js:39
then@resource://gre/modules/commonjs/sdk/core/promise.js:128
Templater.prototype._handleAsync@resource:///modules/devtools/Templater.jsm:458
Templater.prototype._processForEach@resource:///modules/devtools/Templater.jsm:282
Templater.prototype.processNode@resource:///modules/devtools/Templater.jsm:116
Templater.prototype.processNode@resource:///modules/devtools/Templater.jsm:204
template@resource:///modules/devtools/Templater.jsm:48
Completer.prototype.update@resource:///modules/devtools/gcli.jsm:10506
exports.createEvent/event@resource:///modules/devtools/gcli.jsm:666
Inputter.prototype._checkAssignment@resource:///modules/devtools/gcli.jsm:10026
Inputter.prototype._processCaretChange@resource:///modules/devtools/gcli.jsm:10006
Inputter.prototype.textChanged@resource:///modules/devtools/gcli.jsm:9919
exports.createEvent/event@resource:///modules/devtools/gcli.jsm:666
Requisition.prototype.update/</<@resource:///modules/devtools/gcli.jsm:6712
effort@resource://gre/modules/commonjs/sdk/core/promise.js:57
resolved@resource://gre/modules/commonjs/sdk/core/promise.js:122
then@resource://gre/modules/commonjs/sdk/core/promise.js:39
then@resource://gre/modules/commonjs/sdk/core/promise.js:128
Requisition.prototype.update/<@resource:///modules/devtools/gcli.jsm:6713
effort@resource://gre/modules/commonjs/sdk/core/promise.js:57
resolved@resource://gre/modules/commonjs/sdk/core/promise.js:122
then@resource://gre/modules/commonjs/sdk/core/promise.js:39
then@resource://gre/modules/commonjs/sdk/core/promise.js:128
Requisition.prototype.update@resource:///modules/devtools/gcli.jsm:6714
Inputter.prototype.setInput@resource:///modules/devtools/gcli.jsm:10051
helpers.setInput@chrome://mochitests/content/browser/browser/devtools/commandline/test/helpers.js:450
helpers._setup@chrome://mochitests/content/browser/browser/devtools/commandline/test/helpers.js:760
helpers.audit/<@chrome://mochitests/content/browser/browser/devtools/commandline/test/helpers.js:915
exports.promiseEach/callNext@resource:///modules/devtools/gcli.jsm:904
exports.promiseEach/callNext/<@resource:///modules/devtools/gcli.jsm:913
effort@resource://gre/modules/commonjs/sdk/core/promise.js:57
resolved@resource://gre/modules/commonjs/sdk/core/promise.js:122
effort@resource://gre/modules/commonjs/sdk/core/promise.js:57
resolved@resource://gre/modules/commonjs/sdk/core/promise.js:122
effort@resource://gre/modules/commonjs/sdk/core/promise.js:57
resolved@resource://gre/modules/commonjs/sdk/core/promise.js:122
effort@resource://gre/modules/commonjs/sdk/core/promise.js:57
resolved@resource://gre/modules/commonjs/sdk/core/promise.js:122
effort@resource://gre/modules/commonjs/sdk/core/promise.js:57
resolved@resource://gre/modules/commonjs/sdk/core/promise.js:122
then@resource://gre/modules/commonjs/sdk/core/promise.js:39
then@resource://gre/modules/commonjs/sdk/core/promise.js:128
then@resource://gre/modules/commonjs/sdk/core/promise.js:128
then@resource://gre/modules/commonjs/sdk/core/promise.js:128
then@resource://gre/modules/commonjs/sdk/core/promise.js:128
then@resource://gre/modules/commonjs/sdk/core/promise.js:128
exports.promiseEach/callNext@resource:///modules/devtools/gcli.jsm:905
exports.promiseEach/callNext/<@resource:///modules/devtools/gcli.jsm:913
effort@resource://gre/modules/commonjs/sdk/core/promise.js:57
resolved@resource://gre/modules/commonjs/sdk/core/promise.js:122
effort@resource://gre/modules/commonjs/sdk/core/promise.js:57
resolved@resource://gre/modules/commonjs/sdk/core/promise.js:122
effort@resource://gre/modules/commonjs/sdk/core/promise.js:57
resolved@resource://gre/modules/commonjs/sdk/core/promise.js:122
effort@resource://gre/modules/commonjs/sdk/core/promise.js:57
resolved@resource://gre/modules/commonjs/sdk/core/promise.js:122
effort@resource://gre/modules/commonjs/sdk/core/promise.js:57
resolved@resource://gre/modules/commonjs/sdk/core/promise.js:122
then@resource://gre/modules/commonjs/sdk/core/promise.js:39
then@resource://gre/modules/commonjs/sdk/core/promise.js:128
then@resource://gre/modules/commonjs/sdk/core/promise.js:128
then@resource://gre/modules/commonjs/sdk/core/promise.js:128
then@resource://gre/modules/commonjs/sdk/core/promise.js:128
then@resource://gre/modules/commonjs/sdk/core/promise.js:128
exports.promiseEach/callNext@resource:///modules/devtools/gcli.jsm:905
exports.promiseEach/callNext/<@resource:///modules/devtools/gcli.jsm:913
effort@resource://gre/modules/commonjs/sdk/core/promise.js:57
resolved@resource://gre/modules/commonjs/sdk/core/promise.js:122
effort@resource://gre/modules/commonjs/sdk/core/promise.js:57
resolved@resource://gre/modules/commonjs/sdk/core/promise.js:122
effort@resource://gre/modules/commonjs/sdk/core/promise.js:57
resolved@resource://gre/modules/commonjs/sdk/core/promise.js:122
effort@resource://gre/modules/commonjs/sdk/core/promise.js:57
resolved@resource://gre/modules/commonjs/sdk/core/promise.js:122
effort@resource://gre/modules/commonjs/sdk/core/promise.js:57
resolved@resource://gre/modules/commonjs/sdk/core/promise.js:122
then@resource://gre/modules/commonjs/sdk/core/promise.js:39
then@resource://gre/modules/commonjs/sdk/core/promise.js:128
then@resource://gre/modules/commonjs/sdk/core/promise.js:128
then@resource://gre/modules/commonjs/sdk/core/promise.js:128
then@resource://gre/modules/commonjs/sdk/core/promise.js:128
then@resource://gre/modules/commonjs/sdk/core/promise.js:128
exports.promiseEach/callNext@resource:///modules/devtools/gcli.jsm:905
exports.promiseEach/callNext/<@resource:///modules/devtools/gcli.jsm:913
effort@resource://gre/modules/commonjs/sdk/core/promise.js:57
resolved@resource://gre/modules/commonjs/sdk/core/promise.js:122
effort@resource://gre/modules/commonjs/sdk/core/promise.js:57
resolved@resource://gre/modules/commonjs/sdk/core/promise.js:122
effort@resource://gre/modules/commonjs/sdk/core/promise.js:57
resolved@resource://gre/modules/commonjs/sdk/core/promise.js:122
effort@resource://gre/modules/commonjs/sdk/core/promise.js:57
resolved@resource://gre/modules/commonjs/sdk/core/promise.js:122
effort@resource://gre/modules/commonjs/sdk/core/promise.js:57
resolved@resource://gre/modules/commonjs/sdk/core/promise.js:122
then@resource://gre/modules/commonjs/sdk/core/promise.js:39
then@resource://gre/modules/commonjs/sdk/core/promise.js:128
then@resource://gre/modules/commonjs/sdk/core/promise.js:128
then@resource://gre/modules/commonjs/sdk/core/promise.js:128
then@resource://gre/modules/commonjs/sdk/core/promise.js:128
then@resource://gre/modules/commonjs/sdk/core/promise.js:128
exports.promiseEach/callNext@resource:///modules/devtools/gcli.jsm:905
exports.promiseEach/callNext/<@resource:///modules/devtools/gcli.jsm:913
effort@resource://gre/modules/commonjs/sdk/core/promise.js:57
resolved@resource://gre/modules/commonjs/sdk/core/promise.js:122
effort@resource://gre/modules/commonjs/sdk/core/promise.js:57
resolved@resource://gre/modules/commonjs/sdk/core/promise.js:122
effort@resource://gre/modules/commonjs/sdk/core/promise.js:57
resolved@resource://gre/modules/commonjs/sdk/core/promise.js:122
effort@resource://gre/modules/commonjs/sdk/core/promise.js:57
resolved@resource://gre/modules/commonjs/sdk/core/promise.js:122
effort@resource://gre/modules/commonjs/sdk/core/promise.js:57
resolved@resource://gre/modules/commonjs/sdk/core/promise.js:122
then@resource://gre/modules/commonjs/sdk/core/promise.js:39
then@resource://gre/modules/commonjs/sdk/core/promise.js:128
then@resource://gre/modules/commonjs/sdk/core/promise.js:128
then@resource://gre/modules/commonjs/sdk/core/promise.js:128
then@resource://gre/modules/commonjs/sdk/core/promise.js:128
then@resource://gre/modules/commonjs/sdk/core/promise.js:128
exports.promiseEach/callNext@resource:///modules/devtools/gcli.jsm:905
exports.promiseEach/callNext/<@resource:///modules/devtools/gcli.jsm:913
effort@resource://gre/modules/commonjs/sdk/core/promise.js:57
resolved@resource://gre/modules/commonjs/sdk/core/promise.js:122
effort@resource://gre/modules/commonjs/sdk/core/promise.js:57
resolved@resource://gre/modules/commonjs/sdk/core/promise.js:122
effort@resource://gre/modules/commonjs/sdk/core/promise.js:57
resolved@resource://gre/modules/commonjs/sdk/core/promise.js:122
effort@resource://gre/modules/commonjs/sdk/core/promise.js:57
resolved@resource://gre/modules/commonjs/sdk/core/promise.js:122
effort@resource://gre/modules/commonjs/sdk/core/promise.js:57
resolved@resource://gre/modules/commonjs/sdk/core/promise.js:122
then@resource://gre/modules/commonjs/sdk/core/promise.js:39
then@resource://gre/modules/commonjs/sdk/core/promise.js:128
then@resource://gre/modules/commonjs/sdk/core/promise.js:128
then@resource://gre/modules/commonjs/sdk/core/promise.js:128
then@resource://gre/modules/commonjs/sdk/core/promise.js:128
then@resource://gre/modules/commonjs/sdk/core/promise.js:128
exports.promiseEach/callNext@resource:///modules/devtools/gcli.jsm:905
exports.promiseEach/callNext/<@resource:///modules/devtools/gcli.jsm:913
effort@resource://gre/modules/commonjs/sdk/core/promise.js:57
resolved@resource://gre/modules/commonjs/sdk/core/promise.js:122
effort@resource://gre/modules/commonjs/sdk/core/promise.js:57
resolved@resource://gre/modules/commonjs/sdk/core/promise.js:122
effort@resource://gre/modules/commonjs/sdk/core/promise.js:57
resolved@resource://gre/modules/commonjs/sdk/core/promise.js:122
effort@resource://gre/modules/commonjs/sdk/core/promise.js:57
resolved@resource://gre/modules/commonjs/sdk/core/promise.js:122
effort@resource://gre/modules/commonjs/sdk/core/promise.js:57
resolved@resource://gre/modules/commonjs/sdk/core/promise.js:122
then@resource://gre/modules/commonjs/sdk/core/promise.js:39
then@resource://gre/modules/commonjs/sdk/core/promise.js:128
then@resource://gre/modules/commonjs/sdk/core/promise.js:128
then@resource://gre/modules/commonjs/sdk/core/promise.js:128
then@resource://gre/modules/commonjs/sdk/core/promise.js:128
then@resource://gre/modules/commonjs/sdk/core/promise.js:128
exports.promiseEach/callNext@resource:///modules/devtools/gcli.jsm:905
exports.promiseEach/callNext/<@resource:///modules/devtools/gcli.jsm:913
effort@resource://gre/modules/commonjs/sdk/core/promise.js:57
resolved@resource://gre/modules/commonjs/sdk/core/promise.js:122
effort@resource://gre/modules/commonjs/sdk/core/promise.js:57
resolved@resource://gre/modules/commonjs/sdk/core/promise.js:122
effort@resource://gre/modules/commonjs/sdk/core/promise.js:57
resolved@resource://gre/modules/commonjs/sdk/core/promise.js:122
effort@resource://gre/modules/commonjs/sdk/core/promise.js:57
resolved@resource://gre/modules/commonjs/sdk/core/promise.js:122
effort@resource://gre/modules/commonjs/sdk/core/promise.js:57
resolved@resource://gre/modules/commonjs/sdk/core/promise.js:122
then@resource://gre/modules/commonjs/sdk/core/promise.js:39
then@resource://gre/modules/commonjs/sdk/core/promise.js:128
then@resource://gre/modules/commonjs/sdk/core/promise.js:128
then@resource://gre/modules/commonjs/sdk/core/promise.js:128
then@resource://gre/modules/commonjs/sdk/core/promise.js:128
then@resource://gre/modules/commonjs/sdk/core/promise.js:128
exports.promiseEach/callNext@resource:///modules/devtools/gcli.jsm:905
exports.promiseEach@resource:///modules/devtools/gcli.jsm:920
helpers.audit@chrome://mochitests/content/browser/browser/devtools/commandline/test/helpers.js:874
tests.gatTest/onGatReady@chrome://mochitests/content/browser/browser/devtools/commandline/test/browser_cmd_addon.js:107
addonAsync@resource:///modules/devtools/BuiltinCommands.jsm:304
safeCall@resource://gre/modules/AddonManager.jsm:81
getAddonsByTypes_noMoreObjects@resource://gre/modules/AddonManager.jsm:1777
AOC_callNext@resource://gre/modules/AddonManager.jsm:176
getAddonsByTypes_concatAddons@resource://gre/modules/AddonManager.jsm:1772
PL_getAddonsByTypes@resource://gre/modules/PluginProvider.jsm:149
callProvider@resource://gre/modules/AddonManager.jsm:107
getAddonsByTypes_nextObject@resource://gre/modules/AddonManager.jsm:1770
AOC_callNext@resource://gre/modules/AddonManager.jsm:182
getAddonsByTypes_concatAddons@resource://gre/modules/AddonManager.jsm:1772
LightweightThemeManager_getAddonsByTypes@resource://gre/modules/LightweightThemeManager.jsm:386
callProvider@resource://gre/modules/AddonManager.jsm:107
getAddonsByTypes_nextObject@resource://gre/modules/AddonManager.jsm:1770
AOC_callNext@resource://gre/modules/AddonManager.jsm:182
getAddonsByTypes_concatAddons@resource://gre/modules/AddonManager.jsm:1772
getAddonsByTypes_getVisibleAddons@resource://gre/modules/XPIProvider.jsm:3283
completeAddon@resource://gre/modules/XPIProvider.jsm -> resource://gre/modules/XPIProviderUtils.js:157
AddonRepo_getCachedAddonByID@resource://gre/modules/AddonRepository.jsm:518
handleResult_makeAddonFromRowAsync@resource://gre/modules/XPIProvider.jsm -> resource://gre/modules/XPIProviderUtils.js:161
XPIDB_fetchAddonMetadata/readTargetPlatforms/readTargetPlatforms_handleCompletion/<@resource://gre/modules/XPIProvider.jsm -> resource://gre/modules/XPIProviderUtils.js:1239
readTargetPlatforms_handleCompletion@resource://gre/modules/XPIProvider.jsm -> resource://gre/modules/XPIProviderUtils.js:1238
|
||
Comment 1•12 years ago
|
||
This issue also introduce extremelly long stack when various error happen in the SDK,
that gives horrible stack trace.
For example here is one issue reported today:
https://github.com/brianking/ReMo-Helper/issues/25
We are also using promises during addon startup, we may eventually be facing similar breakages to the one described by Joe during a critical path.
Can we eventually triage this bug with an higher priority?
Flags: needinfo?(kwierso)
Can do.
Flags: needinfo?(kwierso)
Priority: P3 → --
|
Reporter | |
Comment 3•12 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.
|
||
Comment 4•12 years ago
|
||
See bug 845842 where promises and tasks might be causing FHR to exhaust stack space.
|
|
Reporter | |
Comment 5•12 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.
Irakli said he'd like to tackle this in the work week.
Flags: needinfo?(rFobic)
Priority: -- → P2
|
Assignee | |
Comment 7•12 years ago
|
||
Joe, maybe we can do it during work week ?
Flags: needinfo?(rFobic)
|
|
Reporter | |
Comment 8•12 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•12 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.
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)
|
||
Comment 11•12 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?
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)
|
|
||
Comment 13•12 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.
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•12 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
|
|
Reporter | |
Comment 16•12 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?
I will try and provide a setTimeout() version within the next few days, after Mossop has resolved the regression above.
|
|
Reporter | |
Comment 18•12 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.
|
|
||
Comment 19•12 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•12 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•12 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•12 years ago
|
||
Irakli - I think this is a serious issue, what do you think we should do?
Flags: needinfo?(rFobic)
|
|
Assignee | |
Updated•12 years ago
|
Assignee: nobody → rFobic
|
|
Assignee | |
Comment 23•12 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•12 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•12 years ago
|
||
Pointer to Github pull-request
|
|
Assignee | |
Updated•12 years ago
|
Attachment #722522 -
Flags: review?(dtownsend+bugmail)
Attachment #722522 -
Flags: feedback?(jwalker)
|
|
||
Updated•12 years ago
|
Attachment #722522 -
Flags: review?(dtownsend+bugmail) → review+
|
||
Comment 26•12 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?
|
|
||
Comment 27•12 years ago
|
||
https://developer.mozilla.org/en-US/docs/Components.stack ?
stack after - stack before < reasonable stack count
|
|
Reporter | |
Updated•12 years ago
|
Attachment #722522 -
Flags: feedback?(jwalker) → feedback+
|
|
Reporter | |
Comment 28•12 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.
|
||
Comment 29•12 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
|
|
Assignee | |
Comment 30•12 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.
Flags: needinfo?(jwalker)
|
|
||
Comment 31•12 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
|
|
Reporter | |
Comment 32•12 years ago
|
||
I can confirm that this patch fixed this problem. Sorry it took me a long time to create a patch to remove my workarounds.
Status: NEW → RESOLVED
Closed: 12 years ago
Flags: needinfo?(jwalker)
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•