Handle jQuery polymorphic selector argument better

RESOLVED FIXED

Status

()

Core
JavaScript Engine: JIT
P3
normal
Rank:
37
RESOLVED FIXED
5 years ago
5 months ago

People

(Reporter: jandem, Unassigned)

Tracking

(Blocks: 3 bugs, {perf})

unspecified
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(platform-rel +)

Details

(Whiteboard: [platform-rel-jQuery][qf:f60][qf:p1])

(Reporter)

Description

5 years ago
jQuery.fn.init is the main jQuery function. It starts like this (v1.6.4):

    if ( !selector ) {
        return this;
    }

    // Handle $(DOMElement)
    if ( selector.nodeType ) {
        ...
    }

    if ( selector === "body" && !context && document.body ) {
        ...
    }

    // Handle HTML strings
    if ( typeof selector === "string" ) {
        ...
    }

The problem is that jQuery itself calls this function and passes (1) document and (2) a function as selector argument. Then we run the benchmark and selector is a string 100% of the time. If we could compile this function and assume selector is a string, we could make most of these checks a lot faster (optimize away the typeof selector === "string", if (selector.nodeType) etc).

Usually we start gathering type information when the script is Baseline compiled so we wouldn't have this problem I think. Unfortunately this function is a constructor so we collect type information immediately (definite properties analysis).
Hmm.... jQuery.fn.init a constructor because of this bit:

	jQuery = function( selector, context ) {
		// The jQuery object is actually just the init constructor 'enhanced'
		return new jQuery.fn.init( selector, context, rootjQuery );
	},

?
Well, the 'return this' early in the script means there won't be any definite properties.  A lame way to handle this would be to sniff for that bytecode pattern early in the script before any other uses of 'this' and don't bother analyzing (or baseline compiling) in that case.  An alternative would be to allow the analysis to run without baseline compiling the script first (and only baseline compile if definite properties were found), though that is tricky since the bytecode type map (which will be needed) is currently stored on the baseline script.
Whiteboard: [platform-rel-jQuery]

Updated

2 years ago
platform-rel: --- → ?
platform-rel: ? → +

Updated

2 years ago
Rank: 37
Is this still something we'd like to do, Jan?
Flags: needinfo?(jdemooij)
(Reporter)

Comment 4

a year ago
We should be doing much better here now because our ICs now know how to optimize this part in comment 0 when selector is a string:

    if ( selector.nodeType ) {

I'm working on other IC optimizations atm, let's keep this open a bit longer and remeasure later.
Blocks: 1307062
Component: JavaScript Engine → JavaScript Engine: JIT
Flags: needinfo?(jdemooij)
Priority: -- → P3
Keywords: perf
Whiteboard: [platform-rel-jQuery] → [platform-rel-jQuery][qf]

Comment 5

a year ago
Kannan, what to do with this bug also?
Flags: needinfo?(kvijayan)
p1.
Flags: needinfo?(kvijayan)
Whiteboard: [platform-rel-jQuery][qf] → [platform-rel-jQuery][qf:p1]
(In reply to Jan de Mooij [:jandem] from comment #4)
> We should be doing much better here now because our ICs now know how to
> optimize this part in comment 0 when selector is a string:
> 
>     if ( selector.nodeType ) {
> 
> I'm working on other IC optimizations atm, let's keep this open a bit longer
> and remeasure later.

Is it possible to remeasure now?
Flags: needinfo?(jdemooij)

Comment 8

9 months ago
This doesn't seem like it will be fixed by 57 so I moving it to P2 for post 57 work.
Whiteboard: [platform-rel-jQuery][qf:p1] → [platform-rel-jQuery][qf:p2]
Jan have you remeasured this?
Whiteboard: [platform-rel-jQuery][qf:p2] → [platform-rel-jQuery][perf:p1]
Whiteboard: [platform-rel-jQuery][perf:p1] → [platform-rel-jQuery][qf:p1]

Updated

7 months ago
Whiteboard: [platform-rel-jQuery][qf:p1] → [platform-rel-jQuery][qf:i60][qf:p1]

Updated

6 months ago
Whiteboard: [platform-rel-jQuery][qf:i60][qf:p1] → [platform-rel-jQuery][qf:f60][qf:p1]
(Reporter)

Comment 10

5 months ago
As mentioned in comment 4, we should do much better here nowadays. It has been 4 years and we don't have a testcase for this particular bug, so I think I'll just close this and we can file new bugs if we see more slowness here.
Status: NEW → RESOLVED
Last Resolved: 5 months ago
Flags: needinfo?(jdemooij)
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.