The committers listed above are authorized under a signed CLA.

• ✅ login: ac-mmi (fec06b9, 406d0b8, 903961e, 464d9f5, 8226069, 704e68f, 4b918ee, 324e269, 65d6234, f69df14, 54ea9c4, 5fdb938, 13fd023, d23bcbc, a770892)
• ✅ login: mgol / name: Michał Gołębiowski-Owczarek (1e7c4e6, 6f061c6)

@mgol Done

@mgol All tests passed done.

Most of my last review still applies.

@mgol Done changed getAll.js and all tests passed

@mgol I added tests that specifically address the issues I aim to fix and conducted a performance test on the updated getAll() functionality using 1000 <span></span> items. Here are the results:

Original implementation time: 1 ms
New implementation time: 6.5 ms

@ac-mmi can you share the test code, please?

@mgol added the test for the issue addressing #5214

@mgol Code for measuring performance

function measurePerformance(fn, context, tag, iterations = 1000) {
    var startTime = performance.now();
    for (let i = 0; i < iterations; i++) {
        fn(context, tag);
    }
    var endTime = performance.now();
    return endTime - startTime;
}

var context = document.createElement("div");
context.innerHTML = "<span></span>".repeat(1000);

// Original implementation using getElementsByTagName
function originalGetAll(context, tag) {
    var ret;
    if (typeof context.getElementsByTagName !== "undefined") {
        ret = context.getElementsByTagName(tag || "*");
    } else if (typeof context.querySelectorAll !== "undefined") {
        ret = context.querySelectorAll(tag || "*");
    } else {
        ret = [];
    }

    if (tag === undefined || (tag && nodeName(context, tag))) {
        return jQuery.merge([context], ret);
    }

    return ret;
}

// New implementation using querySelectorAll
function newGetAll(context, tag) {
    var ret;
    if (typeof context.querySelectorAll !== "undefined") {
        ret = context.querySelectorAll(tag || "*");
    } else {
        ret = [];
    }

    if (tag === undefined || (tag && nodeName(context, tag))) {
        return jQuery.merge([context], ret);
    }

    return ret;
}

function nodeName( elem, name ) {
	return elem.nodeName && elem.nodeName.toLowerCase() === name.toLowerCase();
}

console.log("New implementation time: " + measurePerformance(newGetAll, context, "span") + "ms");

console.log("Original implementation time: " + measurePerformance(originalGetAll, context, "span") + "ms");

Construction of a perf test

@ac-mmi the perf tests you provided test just raw performance of various versions of getAll. However, what's more important is its effect on real jQuery APIs. Most of getAll usage is meant to just find scripts which are usually limited in numbers. However, two methods: remove and empty - use it to find all the nodes within the current one.

I created a test case using three versions of jQuery:

1. A real jQuery Git one.
2. With a patched getAll to remove the getElementsByTagName path.
3. With a patched getAll to wrap the getElementsByTagName path with an additional slice call:

   if ( typeof context.getElementsByTagName !== "undefined" ) {
   	ret = arr.slice.call(context.getElementsByTagName( tag || "*" ));

   (arr is https://github.com/jquery/jquery/blob/dbc9dac7aecb106b66050342ff8daf1ecdd4239f/src/var/arr.js)

The first version is the fastest but the third one is faster than the second one. The differences between (2) & (3) in Chrome & Safari are small enough that I'd opt to (2), but in Firefox they are massive.

Test case: https://jsperf.app/gifera/1/preview

Environment

Tests done on macOS Sonoma 14.6.1 (23G93) on a MacBook Pro with the M3 Max chip.

Browsers:

1. Chrome 127.0.6533.100
2. Firefox 129.0
3. Safari 17.6 (19618.3.11.11.5)

Results

Conclusions

The perf difference in Firefox is staggering so I'd go with the slice version. That will get us a 15-21% perf cut on empty on large-ish structures, hopefully that's acceptable to fix the issue at hand.

Landed on main in 3cad5c4 & on 3.x-stable in 752e911. Thanks!