966264 - Consider adding common names for "[object Object]" etc

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jan de Mooij [:jandem]]

Noticed this in a Octane-box2d profile (see bug 966259 for more info).

JS_BasicObjectToString returns a new "[object X]" JSString where X is the class name. It may be worthwhile to special-case the most common cases, "[object Object]", "[object Array]" etc and return static atoms.

I'll instrument the browser to see if we allocate many of these strings, and which ones are most common.

Comment 1

[Jan de Mooij [:jandem]]

I added a printf in JS_BasicObjectToString, created a new profile and opened 10 popular websites:

- GMail, login, open a random mail
- Amazon
- Facebook (no login)
- twitter.com/firefox
- outlook.com (no login)
- maps.google.com
- en.wikipedia.org/wiki/Firefox
- TechCrunch
- reddit
- eBay

Here's the list of classes with more than 10 hits:

  12 class: HTMLCollection
  20 class: CABI
  28 class: HTMLDivElement
  75 class: NodeList
  84 class: RegExp
 138 class: Arguments
 186 class: Boolean
 214 class: Generator
 272 class: Text
 809 class: Number   ( 5%)
 861 class: Window   ( 5%)
1615 class: Function (10%)
2745 class: Array    (17%)
4160 class: String   (26%)
5055 class: Object   (31%)

Total: 16274

Object, String, Array and Function are responsible for 84% of the strings. CC'ing njn as this may be interesting from a memory usage POV.

Comment 2

[Andrew McCreight [:mccr8]]

njn saw a ridiculous number of this particular string when investigated pdf.js.  They went away pretty quickly, but it would probably still be better if we could avoid them.

Comment 3

[Jan de Mooij [:jandem]]

Attachment: Patch

Special-cases Object, String, Array, Function and Number.

Comment 4

[Nicholas Nethercote [inactive]]

Comment on attachment 8368622 [details] [diff] [review]
Patch

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

Yeah, I saw 10s of thousands of "[object Object]" in pdf.js. Have you been reading IRC scrollback? :)

Can you do "[object Window]" as well, or would that involve changes outside of SpiderMonkey?

Comment 5

[Tom S. (please needinfo tschuster)]

I guess we could also call js::Atomize on the other strings? Class names should usually be relatively short.

Comment 6

[Jan de Mooij [:jandem]]

(In reply to Nicholas Nethercote [:njn] from comment #4)
> Yeah, I saw 10s of thousands of "[object Object]" in pdf.js. Have you been
> reading IRC scrollback? :)

No I was looking at an Octane-box2d profile and noticed ~8% of time under DefaultValue and a lot of that under obj_toString; see also bug 966259 for the details. I just looked at the IRC log though, that's a pretty weird coincidence :(

> Can you do "[object Window]" as well, or would that involve changes outside
> of SpiderMonkey?

If you think strcmp(className, "Window") == 0 is acceptable..

(In reply to Tom Schuster [:evilpie] from comment #5)
> I guess we could also call js::Atomize on the other strings? Class names
> should usually be relatively short.

That's not a bad idea; we could do that as a followup.

Comment 7

[Nicholas Nethercote [inactive]]

> > Can you do "[object Window]" as well, or would that involve changes outside
> > of SpiderMonkey?
> 
> If you think strcmp(className, "Window") == 0 is acceptable..

Seems ok, but I'll let you decide.

Comment 8

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/02302b3cdd84

(In reply to Nicholas Nethercote [:njn] from comment #7)
> > If you think strcmp(className, "Window") == 0 is acceptable..
> 
> Seems ok, but I'll let you decide.

Done. We've other references to Window in js/src and even a strcmp(obj->getClass()->name, "Location"), so it seems fine.

Comment 9

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/02302b3cdd84

Comment 10

[Jeff Walden [:Waldo]]

Wait, wait, wait.  "Location" is a temporary hack, allowed because of its temporal nature (it's going to die as Location/Window get WebIDL'd).  It's entirely non-precedential for that reason.

The non-test, Window-in-browser-sense hits in js/src are for window proxies, and in friendapi for nuking cross-compartment wrappers.  It seems to me both cases could/should be generalized to discuss this more in terms of global objects than in terms of windows.  But it also doesn't seem these hits are in the same vein as the change here, so I wouldn't count that against it.

I'm not sure whether I'm opposed to "[object Window]" or not, which for this probably means I'm not opposed.  :-)  I just don't think our existing hacks should be considered justification for adding this particular hack.