Opened 8 years ago

Closed 8 years ago

#13352 closed defect (fixed)

[patch][CLA] Add global function symbols for debugging (missing feature in new builder)

Reported by: liucougar Owned by: Rawld Gill
Priority: high Milestone: 1.7
Component: BuildSystem Version: 1.7.0b1
Keywords: Cc:
Blocked By: Blocking:

Description

this feature was introduced to the old builder in #5341

it's missing in the new builder

a proposed patch is incoming

Attachments (3)

11352.patch (6.3 KB) - added by liucougar 8 years ago.
propsed patch to add inserting symbol support
11352.2.patch (6.7 KB) - added by liucougar 8 years ago.
version 2: fixed a bug in depScan transform
11352.3.patch (7.2 KB) - added by liucougar 8 years ago.
third version

Download all attachments as: .zip

Change History (12)

Changed 8 years ago by liucougar

Attachment: 11352.patch added

propsed patch to add inserting symbol support

Changed 8 years ago by liucougar

Attachment: 11352.2.patch added

version 2: fixed a bug in depScan transform

comment:1 Changed 8 years ago by liucougar

just noticed an issue with the first version of the patch: depScan does not convert legacy provide/require format to AMD format any more when used with inserting symbol support

the issue is actually in depScan (it will do nothing if some other transform calls resource.getText() before depScan does). the second version fixes that in depScan

in addition, the second version of the patch improves the generateSym function

Changed 8 years ago by liucougar

Attachment: 11352.3.patch added

third version

comment:2 Changed 8 years ago by liucougar

attached a third version: more robust handling of some corner cases when assigning long symbols

comment:3 Changed 8 years ago by liucougar

rcgill, any comment please?

comment:4 Changed 8 years ago by Rawld Gill

Resolution: fixed
Status: newclosed

fixed in [25982]; Thanks cougar!

comment:5 Changed 8 years ago by Bryan Forbes

Resolution: fixed
Status: closedreopened

The functionality introduced in these patches introduces named function expressions (NFEs) which cause memory leaks on IE (see http://kangax.github.com/nfe/). A named function expression looks like this:

var f = function g(){};

In IE, this will actually create two different functions and assign one to "f" and one to "g". Since they both reference each other through a closure, IE won't garbage collect them. In order for IE to garbage collect them, you have to null out the "named" function:

var f = function g(){};
var g = null;

The leaks are cleared up when the browser refreshes or you close the browser, but this is still a leak on single-page applications since you can't reclaim the memory except by navigating away from the page.

comment:6 Changed 8 years ago by liucougar

this feature is opt-in: if you don't specify symbol option when building, no symbol will be inserted

in addition, according to the webpage you mentioned, the NFEs leak is "relatively insignificant. For 10000 function objects, there would be a ~3MB difference. "

fixing the leak for IE maybe possible, but that would make the output even bigger...

comment:7 in reply to:  6 Changed 8 years ago by Rawld Gill

Replying to liucougar:

Cougar...when I checked your patch, it looked to me like you ported exactly what was in 1.6. Is that correct? If so, then wasn't this an issue in 1.6?

comment:8 Changed 8 years ago by liucougar

yes, it's more or less a direct port. so it was an issue in 1.6 (and any earlier versions)

comment:9 in reply to:  5 Changed 8 years ago by Rawld Gill

Resolution: fixed
Status: reopenedclosed

Replying to BryanForbes:

The functionality introduced [...] cause memory leaks on IE

The implementation has not changed since 1.6, so this has been a problem for a while. It's not critical so I'm punting this for 1.7. I've transferred this specific issue to #13563.

Note: See TracTickets for help on using tickets.