993548 - aggressively free source string in nsScriptLoader::OnStreamComplete()

Status: RESOLVED FIXED

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

Description

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Free load buffer before starting script compile. (v1)

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.

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Nom for 1.3? since we need this for bug 987556 which is a 1.3+ blocker.

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

https://tbpl.mozilla.org/?tree=Try&rev=a3924c737ff1

Comment 4

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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.

Comment 5

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

I filed bug 993591 on a more general solution.

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 7

[Jason Smith [:jsmith]]

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.

Comment 8

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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.

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

(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.

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/99bf9e35c4ed

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/99bf9e35c4ed

Comment 12

[Ryan VanderMeulen [:RyanVM]]

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?

Comment 13

[Ben Kelly [:bkelly, not reviewing]]

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

Comment 14

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 15

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 16

[bhavana bajaj [:bajaj]]

Comment on attachment 8403465 [details] [diff] [review]
Free load buffer before starting script compile. (v1)

Also a=bajaj to land it on aurora.

Comment 17

[Ben Kelly [:bkelly, not reviewing]]

(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.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/ad349d17597b
https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/3c0195d9e34d