Closed
Bug 966264
Opened 10 years ago
Closed 10 years ago
Consider adding common names for "[object Object]" etc
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla29
People
(Reporter: jandem, Assigned: jandem)
References
(Blocks 1 open bug)
Details
(Whiteboard: [MemShrink])
Attachments
(1 file)
2.44 KB,
patch
|
n.nethercote
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•10 years ago
|
||
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•10 years ago
|
||
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.
Assignee | ||
Comment 3•10 years ago
|
||
Special-cases Object, String, Array, Function and Number.
Attachment #8368622 -
Flags: review?(n.nethercote)
Comment 4•10 years ago
|
||
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?
Attachment #8368622 -
Flags: review?(n.nethercote) → review+
Updated•10 years ago
|
Whiteboard: [MemShrink]
Comment 5•10 years ago
|
||
I guess we could also call js::Atomize on the other strings? Class names should usually be relatively short.
Assignee | ||
Comment 6•10 years ago
|
||
(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•10 years ago
|
||
> > 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.
Assignee | ||
Comment 8•10 years ago
|
||
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•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/02302b3cdd84
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Comment 10•10 years ago
|
||
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.
You need to log in
before you can comment on or make changes to this bug.
Description
•