Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Performance issues with object property lookup #245

Open
HarlonWang opened this issue Mar 1, 2024 · 11 comments
Open

Performance issues with object property lookup #245

HarlonWang opened this issue Mar 1, 2024 · 11 comments

Comments

@HarlonWang
Copy link

HarlonWang commented Mar 1, 2024

const a = {}
for (let i = 0; i < 10000000; i++) {
    a[i] = i
}

const start = new Date().getTime()
a[1]
console.log("cost:", (new Date().getTime() - start), "ms")

The cost time between QuickJS and V8 (Node) when running the above code.
quickjs: 83 ms
v8: 1 ms

Object property queries under QuickJS still take a relatively long time, I would like to understand the reason for this performance gap, and whether there are any optimization strategies. Additionally, when I changed the object to an array, I was able to save about half of the time

@morn-0
Copy link

morn-0 commented Mar 4, 2024

quickjs-ng/quickjs#120 ?

@chqrlie
Copy link
Collaborator

chqrlie commented Mar 4, 2024

The cost time between QuickJS and Node when running the above code.
quickjs: 83 ms
node: 1 ms

Which part of the code is measured? The array creation or the array element access? The code fragment seems to suggest a timing of 83ms for just a single access a[1], this is abnormal indeed. I shall investigate.

@HarlonWang
Copy link
Author

The cost time between QuickJS and Node when running the above code.
quickjs: 83 ms
node: 1 ms

Which part of the code is measured? The array creation or the array element access? The code fragment seems to suggest a timing of 83ms for just a single access a[1], this is abnormal indeed. I shall investigate.

yes, a timing of a single access a[1], you can see the code for my time consumption statistics is printed before and after a[1].

@HarlonWang
Copy link
Author

quickjs-ng/quickjs#120 ?

I'm not sure what you mean, but I tried running it with quickjs-ng and the results were pretty similar, even though it used an inline cache.

@saghul
Copy link
Contributor

saghul commented Mar 5, 2024

FWIW, I tested it and got the same results. Even when using the v8 command line with no JIT.

@chqrlie
Copy link
Collaborator

chqrlie commented Mar 5, 2024

This looks like a gc issue or possibly a paging issue.

I get a slow timing of 29ms the first time and 0ms after that.

Edit: indeed a gc issue, see below

@morn-0
Copy link

morn-0 commented Mar 5, 2024

quickjs-ng/quickjs#120 ?

I'm not sure what you mean, but I tried running it with quickjs-ng and the results were pretty similar, even though it used an inline cache.我不确定你的意思,但我尝试使用quickjs-ng运行它,结果非常相似,尽管它使用了内联缓存。

Yeah, I noticed that too. Doesn't seem relevant.

@chqrlie
Copy link
Collaborator

chqrlie commented Mar 6, 2024

I modified the test:

function t() {
    var a0 = Date.now();
    let a = {};
    for (let i = 0; i < 10000000; i++) {
        a[i] = i;
    }
    var a1 = Date.now();
    a[1];
    var a2 = Date.now();
    new Date();
    var a3 = Date.now();
    a = null;
    var a4 = Date.now();
    console.log("costs:", a1 - a0, "ms", a2 - a1, "ms", a3 - a2, "ms", a4 - a3, "ms");
}

t();

The timings are: costs: 667 ms 0 ms 30 ms 20 ms

The problem is unrelated to the object access a[1], the call to new Date() takes 30ms (which is way too long on my laptop).

new Date() creates a Date object and initializes it with the current time, which Date.now() computes the same way. The problem hence is in the object creation. I suspect the problem might be in the shape handing, possibly because the shape of the object pointed to bya is huge so rehashing the shape table might take a long time. (edit: not a problem, rehashing is efficient, as the hash key is stored in the shape, there is no need to recompute it, hence constant time for each object).

@saghul
Copy link
Contributor

saghul commented Mar 6, 2024

While not portable, I guess testing with os.now() would remove the noise Date creates? 2ee6be7

@chqrlie
Copy link
Collaborator

chqrlie commented Mar 6, 2024

While not portable, I guess testing with os.now() would remove the noise Date creates? 2ee6be7

os.now() and Date.now() give the same timings. The slow operation is new Date().

@chqrlie
Copy link
Collaborator

chqrlie commented Mar 6, 2024

I nailed it! Creating a new object ultimately calls JS_NewObjectFromShape which in turn calls js_trigger_gc(ctx->rt, sizeof(JSObject)). This is actually the only place where the gc is triggered. The huge object (10 million named properties) must be iterated as part of the gc and this takes a long time even though none of the properties are objects themselves.

This test is pathological as it constructs a huge object in one long loop without constructing any other object and allocating and reallocating a lot of memory so the next object creation triggers a gc for sure.

We could improve the gc speed by keeping a flag in each object indicating if any of the properties are (or have been) objects references, hence accelerating the scan for arrays and objects with just scalar property values (undefined, null, boolean, numbers, strings...). I am not sure how this would impact the overall performance of the interpreter (just a marginal cost when setting property values, that might be implemented without branching). It should reduce the gc overhead significantly, thereby reducing the lag it can cause in large applications.

GerHobbelt pushed a commit to GerHobbelt/quickjs that referenced this issue May 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants