1424901 - Tens of thousands of string copies on Facebook.com

Status: RESOLVED FIXED

(Web Compatibility :: Site Reports, defect, P1)

Description

[Vladan Djeric (:vladan)]

I see a ton of duplicate string copies in about:memory for Firefox's web content process after loading facebook.com while logged in. It is easily reproducible.

├─────559,584 B (00.09%) ── string(length=9, copies=23316, "_5zft img")/gc-heap/latin1
├─────545,192 B (00.09%) -- string(length=24, copies=11815, "Event listenHandler load")
│     ├──283,560 B (00.04%) ── gc-heap/latin1
│     └──261,632 B (00.04%) ── malloc-heap/latin1
├─────284,952 B (00.04%) ── string(length=25, copies=11873, "Event listenHandler error")/gc-heap/latin1
├─────284,952 B (00.04%) ── string(length=26, copies=11873, "DOMEventListener.add error")/gc-heap/latin1
├─────283,560 B (00.04%) ── string(length=25, copies=11815, "DOMEventListener.add load")/gc-heap/latin1
├──────82,160 B (00.01%) -- string(length=7489, copies=10, "q@https://www.facebook.com/:3:4725/na/o.prototype.componentDidMount@https://www.facebook.com/rsrc.php/v3i6FQ4/yn/l/en_US/nge0bBJfXP0.js:623:917/nH@https://www.facebook.com/rsrc.php/v3i6FQ4/yn/l/en_US/nge0bBJfXP0.js:142:65574/nL@https://www.facebook.com/rsrc.php/v3i6FQ4/yn/l/en_US/nge0bBJfXP0.js:142:76662/nK@https://www.facebook.com/rsrc.php/v3i6FQ4/yn/l/en_US/nge0bBJfXP0.js:142:75267/nF@https://www.facebook.com/rsrc.php/v3i6FQ4/yn/l/en_US/nge0bBJfXP0.js:142:73994/nB@https://www.facebook.com/rsrc.php/v3i6FQ4/yn/l/en_US/nge0bBJfXP0.js:142:73217/nz@https://www.facebook.com/rsrc.php/v3i6FQ4/yn/l/en_US/nge0bBJfXP0.js:142:72750/np@https://www.facebook.com/rsrc.php/v3i6FQ4/yn/l/en_US/nge0bBJfXP0.js:142:80028/nu@https://www.facebook.com/rsrc.php/v3i6FQ4/yn/l/en_US/nge0bBJfXP0.js:142:80530/nba/wg.prototype.render@https://www.facebook.com/rsrc.php/v3i6FQ4/yn/l/en_US/nge0bBJfXP0.js:142:96805/nAg/<@https://www.facebook.com/rsrc.php/v3i6FQ4/yn/l/en_US/nge0bBJfXP0.js:142:102965/nV@https://www.facebook.com/rsrc.php/v3i6FQ4" (truncated))
│      ├──81,920 B (00.01%) ── malloc-heap/latin1
│      └─────240 B (00.00%) ── gc-heap/latin1



As you can see above, both short and long strings are duplicated.
There is a similar report in bug 1344503.

Is there a pattern for creating these strings that Facebook code should avoid? Or can you de-dupe these strings yourselves?

Comment 1

[Vladan Djeric (:vladan)]

Just wanted to mention that I looked a little bit more into the about:memory report above and it looks like the duplicate strings are coming from string concatenation in a frequently invoked listener function in our JS code.

De-dup during compacting definitely seems like a good idea.

Comment 2

[Steve Fink [:sfink] [:s:]]

If I ever manage to land nursery strings, another possibility that would work well for something like this is deduplication during tenuring.

Comment 3

[Nicholas Nethercote [inactive]]

De-duplication sounds expensive. Can this not be fixed on the Facebook side?

Comment 4

[Jan de Mooij [:jandem]]

(In reply to Nicholas Nethercote [:njn] from comment #3)
> Can this not be fixed on the Facebook side?

Yeah this would be ideal: allocating and then deduplicating this many strings isn't free + it puts more pressure on the GC (heuristics).

Comment 5

[Jason Orendorff [:jorendorff]]

~2MB seems not even worth chasing (though admittedly the problem scales).

On the JS end this is something we would just close, so -> Tech Evangelism. Maybe it is also not worth doing there. (Apologies if this is the wrong component.)

Comment 6

[Karl Dubost💡 :karlcow]

* What is happening in the same scenario in other browsers (Safari, IE, Chrome)?
* What are the steps to reproduce? Just "after loading facebook.com while logged in."
* Do you know which JavaScript in the page creates this?

Comment 7

[Steve Fink [:sfink] [:s:]]

(In reply to Jason Orendorff [:jorendorff] from comment #5)
> ~2MB seems not even worth chasing (though admittedly the problem scales).

As originally reported on IRC, it was 56MB of ":DIV".

Comment 8

[Benoit Girard (:BenWa)]

We'll get as much as possible fixed on the Facebook site since it's silly. Let Facebook know if you see anything bad. But it also means for us shipping more JS to avoid redundant copies which isn't great. At least there's still bug 1344503 left to consider mitigating this at the platform level, it would be interesting to know how widespread this issue is across the web.

Comment 9

[Nathan Froyd [:froydnj]]

(In reply to Benoit Girard (:BenWa) from comment #8)
> We'll get as much as possible fixed on the Facebook site since it's silly.
> Let Facebook know if you see anything bad. But it also means for us shipping
> more JS to avoid redundant copies which isn't great.

Can you expand on this a little bit?  This statement sounds to me like, "we don't want to write more code to make our code more efficient", which seems like a peculiar position.

Comment 10

[Benoit Girard (:BenWa)]

Happy to. Facebook already ships a lot of code which the browser has a lot of trouble quickly parsing and booting more. So in our code say we have something like:

let handler = 'eventHandler: ' + event.type;

We need to replace this with something like:

const gStringTable = {};

....

if (!gStringTable[event.type]) {
  gStringTable[event.type] = 'eventHandler: ' + event.type;
}

let handler = gStringTable[event.type];

So here we're shipping more code and making startup slower to reduce memory. Generally startup cost is more important to websites like Facebook than memory. Also, unlikely a PGO'ed binary application, where the cost of adding a branch like this is very small and does slow down until you execute nearby code and page this in, with JS this extra line is bloating parse/compile and is slowing down the time to start executing the first statement in the application since the JS engine has to parse every byte in a file.

So 3 lines are worth it in this case because where talking about >0.5 MB of memory, the trade off to adding more code to make things more efficient is not a no-brainer.

Comment 11

[Steve Fink [:sfink] [:s:]]

If you don't mind a complete, possibly engine-specific, hack:

  let handler = Object.keys({['eventHandler: ' + event.type]: 0})[0];

will do the same thing using internal data structures. Property names are placed into a lookup table, and you will get the canonical version if you read out a key.

Comment 12

[Oana Arbuzov [:oanaarbuzov]]

Any updates here Benoit Girard?

Comment 13

[Benoit Girard (:BenWa)]

No updates from me, vladan is looking at memory improvements.

Comment 14

[Vladan Djeric (:vladan)]

I landed a fix for this two weeks ago to 50% of users but had to back it out. I'm relanding it today. The 50% is to measure the magnitude of the benefit from fixing this bug (with property atomization suggested in comment 11).
FYI the property atomization trick works in both Firefox and Chrome :)

Comment 15

[Firefox Bug Husbandry Bot]

See bug 1547409. Moving webcompat whiteboard tags to keywords.

Comment 16

[Mike Taylor [:miketaylr]]

Vladan, can we closet his now?

Comment 17

[Vladan Djeric (:vladan)]

(In reply to Mike Taylor [:miketaylr] (PTO July 31 - Aug 8) from comment #16)

Vladan, can we closet his now?

Let's do it

Comment 18

[Karl Dubost💡 :karlcow]

according to the previous comment let's close. Thanks everyone.