Closed Bug 993548 Opened 6 years ago Closed 6 years ago

aggressively free source string in nsScriptLoader::OnStreamComplete()

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla31
blocking-b2g 1.3+
Tracking Status
firefox29 --- wontfix
firefox30 --- fixed
firefox31 --- fixed
b2g-v1.3 --- fixed
b2g-v1.3T --- fixed
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed

People

(Reporter: bkelly, Assigned: bkelly)

References

Details

(Keywords: perf, Whiteboard: [MemShrink])

Attachments

(1 file)

While investigating bug 987556 I found that some of our extreme peak memory usage while loading javascript resources comes from |nsScriptLoader::OnStreamComplete()|.

This code effectively copies the source string to a new buffer when it performs its |ConvertToUTF16()| call.  It waits, however, until js compilation has completed and the underlying nsStreamLoader is released before free'ing up the original source buffer.

The nsStreamLoader supports returning |NS_SUCCESS_ADOPTED_DATA| from the |OnStreamComplete()| call.  The nsScriptLoader could preemptively free the source buffer prior to starting the js compilation and then return this special value.

For the workload in bug 987556 this saves us about 14MB off our peak memory usage.
This is the patch I am currently testing with for bug 987556.

I'm not thrilled with the const_cast, but not sure about if it would be preferable to change the IDL to not require const on the string here.
Assignee: nobody → bkelly
Status: NEW → ASSIGNED
Attachment #8403465 - Flags: review?(khuey)
Nom for 1.3? since we need this for bug 987556 which is a 1.3+ blocker.
blocking-b2g: --- → 1.3?
OS: Gonk (Firefox OS) → All
Hardware: ARM → All
Comment on attachment 8403465 [details] [diff] [review]
Free load buffer before starting script compile. (v1)

Review of attachment 8403465 [details] [diff] [review]:
-----------------------------------------------------------------

NS_Free is what the font loader uses, so I guess it's ok.
Attachment #8403465 - Flags: review?(khuey) → review+
Try is showing a persistent failure on Android 4.0 Opt RC2, but that appears to have been a separate problem just fixed in e2d52a541b50.

I'm going to proceed with landing this on mozilla-inbound.
Ben - Can you give a risk assessment of this patch? Just trying to find out if we can safely take this to help resolve bug 987556 or not.
Flags: needinfo?(bkelly)
This is very low risk and our automated testing of Gecko will cover it well.  If it sticks on trunk we should absolutely take it on the branch.
(In reply to Jason Smith [:jsmith] from comment #7)
> Ben - Can you give a risk assessment of this patch? Just trying to find out
> if we can safely take this to help resolve bug 987556 or not.

I believe this is low risk.  We are essentially just moving up the release from the destructor in nsStreamLoader up one layer to nsScriptLoader.  The ability to pass buffers on to the next layer for ownership already existed.  So its really quite a small change.
Flags: needinfo?(bkelly)
https://hg.mozilla.org/mozilla-central/rev/99bf9e35c4ed
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
blocking-b2g: 1.3? → 1.3+
Please request approval-mozilla-b2g28? on this patch when it is ready for v1.3 uplift. Also, is this something we should consider taking on mozilla-beta for Fx29?
Flags: needinfo?(bkelly)
Comment on attachment 8403465 [details] [diff] [review]
Free load buffer before starting script compile. (v1)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 987556 
User impact if declined: We use 14MB of additional memory trying to run browsermark which is required for partner IOT.
Testing completed: Device tested on v1.5 and v1.3.
Risk to taking this patch (and alternatives if risky): Minimal
String or UUID changes made by this patch: None
Attachment #8403465 - Flags: approval-mozilla-b2g28?
Flags: needinfo?(bkelly)
I don't think this is worth uplifting to mozilla-beta since transient memory spikes do not have the same impact on resource rich environments like desktop.
Since I think triages are focusing more on 1.4 and tarako these days, I'm going to do a ni? to request someone look at the approval flag.
Flags: needinfo?(praghunath)
Comment on attachment 8403465 [details] [diff] [review]
Free load buffer before starting script compile. (v1)

Also a=bajaj to land it on aurora.
Attachment #8403465 - Flags: approval-mozilla-b2g28? → approval-mozilla-b2g28+
Flags: needinfo?(praghunath)
(In reply to bhavana bajaj [:bajaj] from comment #16)
> Also a=bajaj to land it on aurora.

Did you mean b2g28 here?  I would like this for the linked 1.3+ blocked bug.
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.