fix: Request execution time keeps increasing over time when using Parse.Object.extend

[dblythy]

New Pull Request Checklist

• I am not disclosing a vulnerability.
• I am creating this PR in reference to an issue.

Issue Description

new Parse.Object.extend("") takes increasing execution time when ran in a loop, as it seems that the constructor somehow gets deep assigned each time, meaning that it is exponentially called.

Closes: #1683

Approach

First commit shows the issue, with:

 for (let i = 0; i < 100000; i++) {
      // eslint-disable-next-line
      const Parent = ParseObject.extend('Parent');
      // eslint-disable-next-line
      const parent = new Parent();
}

Timing out the CI, taking too long.

With these changes, this function only takes 38ms.

TODOs before merging

• Add tests
• Add changes to documentation (guides, repository pages, in-code descriptions)

[parse-github-assistant]

I will reformat the title to use the proper commit message syntax.

[parse-github-assistant]

Thanks for opening this pull request!

• 🎉 We are excited about your hands-on contribution!

[dblythy]

Testing this on a Parse Server using saveAll, with 500 objects:

Pre-patch:

Post-patch:

[dblythy]

Related: parse-community/parse-server#7036

[dblythy]

Test function:

Parse.Cloud.define("test", async (request) => {
  const testObj = Parse.Object.extend("Temp");
  await new testObj().save();
  return true;
});

autocannon -m POST -H "X-Parse-Application-Id: myAppId" -c 1000 http://localhost:1337/parse/functions/test

Pre patch:

Post patch:

[codecov]

Codecov Report

Base: 99.89% // Head: 99.89% // Increases project coverage by +0.00% 🎉

Coverage data is based on head (0215c5b) compared to base (c63e738).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files

@@           Coverage Diff           @@
##            alpha    #1682   +/-   ##
=======================================
  Coverage   99.89%   99.89%           
=======================================
  Files          61       61           
  Lines        5977     5984    +7     
  Branches     1369     1372    +3     
=======================================
+ Hits         5971     5978    +7     
  Misses          6        6           

Impacted Files	Coverage Δ
src/ParseObject.js	99.89% <100.00%> (+<0.01%)	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[mtrezza]

That's an amazing improvement! Hats off!

Could this have an effect on the general performance of Parse Server if the JS SDK dependency of Parse Server is updated with this fix? I’m thinking if extend is used internally in Parse Server, even if it’s not used explicitly by the developer in their custom code.

[dblythy]

Does this need to get merged into Parse Server to have an effect when calling saveAll from a client

No, unless there are cloud code triggers that uses extend

I’m thinking if extend is used internally in Parse Server

I don't think extend is used at all internally

[mtrezza]

I’ve updated my comment while you posted. If extend is used anywhere in the server code internally (haven’t checked yet), could it affect these parts of the code as well, essentially causing a leak?

[dblythy]

I'm pretty sure the method isn't used internally. I might just double check that this change doesn't break the tests on Parse Server before we merge

[mtrezza]

Sure, thanks for checking

[dblythy]

Tests passing

[mtrezza]

Looks good! This will hopefully address these vague perf issue reports we're received over the years.

From the investigations this seems to go beyond a mere perf improvement and actually fix a bug. It seems that the execution time keeps increasing until the server slows down significantly. So I'll merge this as a bug fix.

[parseplatformorg]

🎉 This change has been released in version 4.0.0-alpha.7

[parseplatformorg]

🎉 This change has been released in version 4.0.1-beta.1

[parseplatformorg]

🎉 This change has been released in version 4.0.1

[vscorpio]

Issue seems to be still existing even in Parse 4.1.0? I just ran into the same issue when calling Parse.Object.extend in a loop and it gets progressively slower in time. I initially thought my queries are poorly written but then I found this.

[dblythy]

@vscorpio can you share some pseudo-code, or write a failing test?

[vscorpio]

@dblythy I have tested an whatever code creating rows gets slower in time. For example the following code:

    // Create a new ticket purchase event similar to the one produced by the SolanaIndexer class.
    const TicketPurchaseEvent = Parse.Object.extend("EVENT_TicketPurchase");
    const ticketPurchaseEvent = new TicketPurchaseEvent();
    ticketPurchaseEvent.set("lottery_id", activeLottery?.get("lottery_id"));
    ticketPurchaseEvent.set(
      "ticket_number",
      latestTicket?.get("ticket_number")
        ? latestTicket?.get("ticket_number") + 1
        : 1
    );
    ticketPurchaseEvent.set("owner", owner);

    // Save the object to the database.
    await ticketPurchaseEvent.save(null, { useMasterKey: true });

Works perfect for the first 10 tries, but after the 10th iteration it gets progressively slower. CPU usage is never above 7%, Memory usage is negligible.

[mtrezza]

I think it should be possible to write a test case for this, and test for example whether the execution time mean decreases more than x% over time.

[dblythy]

I have written this test and have not observed the issue @vscorpio - is there any cloud code / server rate limit or slowdown to consider?

it('execution time should not increase', async () => {

    const activeLottery = new Parse.Object('Lottery');
    activeLottery.set('lottery_id', 'lotteryID')
    const owner = new Parse.User();
    owner.set({username: 'abc', password: '123'});

    await Promise.all([
      activeLottery.save(), owner.signUp()
    ])

    let latestTicket = null;

    for (let i = 0; i < 10000; i++) {

      let start = new Date();
      const TicketPurchaseEvent = Parse.Object.extend("EVENT_TicketPurchase");
      const ticketPurchaseEvent = new TicketPurchaseEvent();
      ticketPurchaseEvent.set("lottery_id", activeLottery?.get("lottery_id"));
      ticketPurchaseEvent.set(
        "ticket_number",
        latestTicket?.get("ticket_number")
          ? latestTicket?.get("ticket_number") + 1
          : 1
      );
      ticketPurchaseEvent.set("owner", owner);

      // Save the object to the database.
      await ticketPurchaseEvent.save(null, { useMasterKey: true });
      latestTicket = ticketPurchaseEvent;
      const end = new Date();
      console.log('duration: ', end.getTime() - start.getTime());
    }
  });

[mtrezza]

Did you verify that this test would have failed pre-#1682? It's not part of the PR so if it hasn't been added to the tests yet and we can verify the test works it would be good to add it.

There are also some caveats regarding code execution time measurement, so maybe rewrite the test using the Node built-in perf API and test the execution time variance between the mean of the first few and last few runs. That would also be a good template for other perf tests in the future.

[dblythy]

Testing with 50,000 requests and averaging execution times per 10,000 requests (using performance.now():

Parse JS SDK 4.0.0 (pre #1682):

0-9999 average duration: 7.8ms
10000-19999 average duration: 25.1ms
20000-29999 average duration: 43.1ms
30000-39999 average duration: 90.9ms
40000-49999 average duration: 148.8ms

Parse JS SDK 4.1.0 (post #1682):

0-9999 average duration: 1.9ms
10000-19999 average duration: 1.9ms
20000-29999 average duration: 2.1ms
30000-39999 average duration: 2.05ms
40000-49999 average duration: 2.1ms

[mtrezza]

Nice, I think you can just calculate the standard deviation of all runs (not of batches) for the test to pass or fail:

pre-𝜎 = 51.02825
post-𝜎 = 0.09165

So the test could fail if 𝜎 >= 0.1
If that turns out to create a flaky test, we can increase it.