Skip to content

perf: avoid cloning args on every pre/post - #45

Merged
vkarpov15 merged 5 commits into
masterfrom
vkarpov15/perf-2
Apr 14, 2026
Merged

vkarpov15 merged 5 commits into
masterfrom
vkarpov15/perf-2

Conversation

@vkarpov15

Copy link
Copy Markdown
Member

Cloning args on every pre/post was a big drain on performance, especially for pre() where we don't have to add callbacks anymore.

Before:

{
  "results": [
    {
      "scenario": "1 hook, 1 arg",
      "iterations": 200000,
      "totalMs": 49.221,
      "avgUsPerExec": 0.246
    },
    {
      "scenario": "5 hooks, 1 arg",
      "iterations": 200000,
      "totalMs": 136.745,
      "avgUsPerExec": 0.684
    },
    {
      "scenario": "10 hooks, 1 arg",
      "iterations": 200000,
      "totalMs": 243.199,
      "avgUsPerExec": 1.216
    },
    {
      "scenario": "5 hooks, 3 args",
      "iterations": 200000,
      "totalMs": 137.098,
      "avgUsPerExec": 0.685
    },
    {
      "scenario": "5 hooks, 3 args + callback",
      "iterations": 200000,
      "totalMs": 135.913,
      "avgUsPerExec": 0.68
    },
    {
      "scenario": "10 hooks, 3 args + callback",
      "iterations": 200000,
      "totalMs": 244.986,
      "avgUsPerExec": 1.225
    }
  ]
}

After:

{
  "results": [
    {
      "scenario": "1 hook, 1 arg",
      "iterations": 200000,
      "totalMs": 34.19,
      "avgUsPerExec": 0.171
    },
    {
      "scenario": "5 hooks, 1 arg",
      "iterations": 200000,
      "totalMs": 45.294,
      "avgUsPerExec": 0.226
    },
    {
      "scenario": "10 hooks, 1 arg",
      "iterations": 200000,
      "totalMs": 64.934,
      "avgUsPerExec": 0.325
    },
    {
      "scenario": "5 hooks, 3 args",
      "iterations": 200000,
      "totalMs": 41.757,
      "avgUsPerExec": 0.209
    },
    {
      "scenario": "5 hooks, 3 args + callback",
      "iterations": 200000,
      "totalMs": 42.56,
      "avgUsPerExec": 0.213
    },
    {
      "scenario": "10 hooks, 3 args + callback",
      "iterations": 200000,
      "totalMs": 61.592,
      "avgUsPerExec": 0.308
    }
  ]
}

With post, the difference is less pronounced because we still have lingering callbacks. But still non-trival, before:

{
  "results": [
    {
      "scenario": "1 hook, 1 arg",
      "iterations": 200000,
      "totalMs": 38.381,
      "avgUsPerExec": 0.192
    },
    {
      "scenario": "5 hooks, 1 arg",
      "iterations": 200000,
      "totalMs": 98.818,
      "avgUsPerExec": 0.494
    },
    {
      "scenario": "10 hooks, 1 arg",
      "iterations": 200000,
      "totalMs": 156.182,
      "avgUsPerExec": 0.781
    },
    {
      "scenario": "5 hooks, 3 args",
      "iterations": 200000,
      "totalMs": 88.307,
      "avgUsPerExec": 0.442
    },
    {
      "scenario": "5 hooks, 3 args + callback",
      "iterations": 200000,
      "totalMs": 98.874,
      "avgUsPerExec": 0.494
    },
    {
      "scenario": "10 hooks, 3 args + callback",
      "iterations": 200000,
      "totalMs": 158.878,
      "avgUsPerExec": 0.794
    }
  ]
}

After:

{
  "results": [
    {
      "scenario": "1 hook, 1 arg",
      "iterations": 200000,
      "totalMs": 45.687,
      "avgUsPerExec": 0.228
    },
    {
      "scenario": "5 hooks, 1 arg",
      "iterations": 200000,
      "totalMs": 81.51,
      "avgUsPerExec": 0.408
    },
    {
      "scenario": "10 hooks, 1 arg",
      "iterations": 200000,
      "totalMs": 128.164,
      "avgUsPerExec": 0.641
    },
    {
      "scenario": "5 hooks, 3 args",
      "iterations": 200000,
      "totalMs": 75.607,
      "avgUsPerExec": 0.378
    },
    {
      "scenario": "5 hooks, 3 args + callback",
      "iterations": 200000,
      "totalMs": 77.089,
      "avgUsPerExec": 0.385
    },
    {
      "scenario": "10 hooks, 3 args + callback",
      "iterations": 200000,
      "totalMs": 121.383,
      "avgUsPerExec": 0.607
    }
  ]
}

This difference is significant enough to show up in Mongoose's saveSimple() benchmark, which we have been striving to optimize.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 4407e9e035

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread index.js Outdated
Comment thread index.js Outdated
Comment on lines +184 to +188
args = error.args;
newArgs = args.slice();
numArgs = args.length;
if (options?.numCallbackParams != null) {
numArgs = options.numCallbackParams;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Rebuild errorArgs after overwrite in error-handler flow

When an error-handling post middleware throws/callbacks Kareem.overwriteResult, this branch updates args and newArgs but then continues without refreshing errorArgs. That means the next error handler is called with stale pre-overwrite arguments, whereas the previous implementation recomputed args per middleware iteration. This can corrupt chained error-handling behavior whenever one handler overwrites the result and a later handler depends on the updated args.

Useful? React with 👍 / 👎.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Optimizes Kareem hook execution performance by reducing per-hook argument cloning in execPre() / execPost(), and adds benchmark scripts to measure the impact.

Changes:

  • Removes per-pre-hook argument cloning in execPre() and calls each pre hook with the current args array.
  • Refactors execPost() to reuse a single newArgs array and callback wrapper across post hooks, rebuilding only when needed (e.g. overwriteResult).
  • Adds benchmarks/execPre.js and benchmarks/execPost.js to measure pre/post hook execution cost.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.

File Description
index.js Removes per-hook arg cloning and refactors post-hook arg/callback handling for performance.
benchmarks/execPre.js Adds a benchmark runner for execPre() scenarios.
benchmarks/execPost.js Adds a benchmark runner for execPost() scenarios.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread index.js
Comment thread index.js Outdated
Comment thread benchmarks/execPre.js Outdated
Comment thread benchmarks/execPost.js Outdated
@vkarpov15

Copy link
Copy Markdown
Member Author

@hasezoey @AbdelrahmanHafez can you please take a look?

@hasezoey hasezoey left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, though consider resolving copilot's suggestions..

Comment thread index.js Outdated
vkarpov15 and others added 3 commits April 14, 2026 10:40
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@vkarpov15
vkarpov15 merged commit f0e5736 into master Apr 14, 2026
8 checks passed
vkarpov15 added a commit to Automattic/mongoose that referenced this pull request Apr 14, 2026
vkarpov15 added a commit to Automattic/mongoose that referenced this pull request Apr 16, 2026
renovate Bot added a commit to LUISDASARTIMANHAS/template-server-express-node-js that referenced this pull request Apr 20, 2026
This PR contains the following updates:

| Package | Change |
[Age](https://docs.renovatebot.com/merge-confidence/) |
[Confidence](https://docs.renovatebot.com/merge-confidence/) |
|---|---|---|---|
| [mongoose](https://mongoosejs.com)
([source](https://redirect.github.com/Automattic/mongoose)) | [`9.4.1` →
`9.5.0`](https://renovatebot.com/diffs/npm/mongoose/9.4.1/9.5.0) |
![age](https://developer.mend.io/api/mc/badges/age/npm/mongoose/9.5.0?slim=true)
|
![confidence](https://developer.mend.io/api/mc/badges/confidence/npm/mongoose/9.4.1/9.5.0?slim=true)
|

---

### Release Notes

<details>
<summary>Automattic/mongoose (mongoose)</summary>

###
[`v9.5.0`](https://redirect.github.com/Automattic/mongoose/blob/HEAD/CHANGELOG.md#950--2026-04-20)

[Compare
Source](https://redirect.github.com/Automattic/mongoose/compare/9.4.1...9.5.0)

\==================

- feat(debug): add `timestamp` option to debug output
[#&#8203;16216](https://redirect.github.com/Automattic/mongoose/issues/16216)
[rejunp](https://redirect.github.com/rejunp)
- feat(query): add `cloneUpdate` option to explicitly disable update
cloning
[#&#8203;16230](https://redirect.github.com/Automattic/mongoose/issues/16230)
[#&#8203;16202](https://redirect.github.com/Automattic/mongoose/issues/16202)
- feat(query): extend `defaults` query option to `find()`
[#&#8203;16226](https://redirect.github.com/Automattic/mongoose/issues/16226)
[sderrow](https://redirect.github.com/sderrow)
- fix(query): avoid cloning update until absolutely necessary to better
support updates with `__proto__`
[#&#8203;16230](https://redirect.github.com/Automattic/mongoose/issues/16230)
[#&#8203;16202](https://redirect.github.com/Automattic/mongoose/issues/16202)
- fix(query): avoid treating documents with a `$set()` method as objects
with a `$set` property when casting updates
[#&#8203;16230](https://redirect.github.com/Automattic/mongoose/issues/16230)
- fix(queryHelpers): pass default options to discriminators
[#&#8203;16227](https://redirect.github.com/Automattic/mongoose/issues/16227)
[#&#8203;16226](https://redirect.github.com/Automattic/mongoose/issues/16226)
- fix(document): handle including and excluding nested paths with
optimistic concurrency
[#&#8203;16177](https://redirect.github.com/Automattic/mongoose/issues/16177)
[#&#8203;16054](https://redirect.github.com/Automattic/mongoose/issues/16054)
- fix(model): throw `ObjectParameterError` in `insertOne()` if doc is
not an object
[#&#8203;16221](https://redirect.github.com/Automattic/mongoose/issues/16221)
[IshitaSingh0822](https://redirect.github.com/IshitaSingh0822)
- fix(cast): preserve reason in `CastError` message after `setModel()`
[#&#8203;16167](https://redirect.github.com/Automattic/mongoose/issues/16167)
[White-Devil2839](https://redirect.github.com/White-Devil2839)
- perf(model): remove unnecessary clone in `findOneAndUpdate()`
[#&#8203;16230](https://redirect.github.com/Automattic/mongoose/issues/16230)
- perf: use kareem 3.3.0
[mongoosejs/kareem#45](https://redirect.github.com/mongoosejs/kareem/issues/45)
[#&#8203;16229](https://redirect.github.com/Automattic/mongoose/issues/16229)
- chore: use TSTyche assertions
[#&#8203;16222](https://redirect.github.com/Automattic/mongoose/issues/16222)
[mrazauskas](https://redirect.github.com/mrazauskas)

</details>

---

### Configuration

📅 **Schedule**: (UTC)

- Branch creation
  - At any time (no schedule defined)
- Automerge
  - At any time (no schedule defined)

🚦 **Automerge**: Enabled.

♻ **Rebasing**: Whenever PR is behind base branch, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR was generated by [Mend Renovate](https://mend.io/renovate/).
View the [repository job
log](https://developer.mend.io/github/LUISDASARTIMANHAS/template-server-express-node-js).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiI0My4xMjMuOCIsInVwZGF0ZWRJblZlciI6IjQzLjEyMy44IiwidGFyZ2V0QnJhbmNoIjoibWFpbiIsImxhYmVscyI6W119-->
antoinezanardi pushed a commit to antoinezanardi/goat-it-api that referenced this pull request Apr 21, 2026
This PR contains the following updates:

| Package | Change |
[Age](https://docs.renovatebot.com/merge-confidence/) |
[Confidence](https://docs.renovatebot.com/merge-confidence/) |
|---|---|---|---|
| [mongoose](https://mongoosejs.com)
([source](https://redirect.github.com/Automattic/mongoose)) | [`^9.4.1`
→ `^9.5.0`](https://renovatebot.com/diffs/npm/mongoose/9.4.1/9.5.0) |
![age](https://developer.mend.io/api/mc/badges/age/npm/mongoose/9.5.0?slim=true)
|
![confidence](https://developer.mend.io/api/mc/badges/confidence/npm/mongoose/9.4.1/9.5.0?slim=true)
|

---

### Release Notes

<details>
<summary>Automattic/mongoose (mongoose)</summary>

###
[`v9.5.0`](https://redirect.github.com/Automattic/mongoose/blob/HEAD/CHANGELOG.md#950--2026-04-20)

[Compare
Source](https://redirect.github.com/Automattic/mongoose/compare/9.4.1...9.5.0)

\==================

- feat(debug): add `timestamp` option to debug output
[#&#8203;16216](https://redirect.github.com/Automattic/mongoose/issues/16216)
[rejunp](https://redirect.github.com/rejunp)
- feat(query): add `cloneUpdate` option to explicitly disable update
cloning
[#&#8203;16230](https://redirect.github.com/Automattic/mongoose/issues/16230)
[#&#8203;16202](https://redirect.github.com/Automattic/mongoose/issues/16202)
- feat(query): extend `defaults` query option to `find()`
[#&#8203;16226](https://redirect.github.com/Automattic/mongoose/issues/16226)
[sderrow](https://redirect.github.com/sderrow)
- fix(query): avoid cloning update until absolutely necessary to better
support updates with `__proto__`
[#&#8203;16230](https://redirect.github.com/Automattic/mongoose/issues/16230)
[#&#8203;16202](https://redirect.github.com/Automattic/mongoose/issues/16202)
- fix(query): avoid treating documents with a `$set()` method as objects
with a `$set` property when casting updates
[#&#8203;16230](https://redirect.github.com/Automattic/mongoose/issues/16230)
- fix(queryHelpers): pass default options to discriminators
[#&#8203;16227](https://redirect.github.com/Automattic/mongoose/issues/16227)
[#&#8203;16226](https://redirect.github.com/Automattic/mongoose/issues/16226)
- fix(document): handle including and excluding nested paths with
optimistic concurrency
[#&#8203;16177](https://redirect.github.com/Automattic/mongoose/issues/16177)
[#&#8203;16054](https://redirect.github.com/Automattic/mongoose/issues/16054)
- fix(model): throw `ObjectParameterError` in `insertOne()` if doc is
not an object
[#&#8203;16221](https://redirect.github.com/Automattic/mongoose/issues/16221)
[IshitaSingh0822](https://redirect.github.com/IshitaSingh0822)
- fix(cast): preserve reason in `CastError` message after `setModel()`
[#&#8203;16167](https://redirect.github.com/Automattic/mongoose/issues/16167)
[White-Devil2839](https://redirect.github.com/White-Devil2839)
- perf(model): remove unnecessary clone in `findOneAndUpdate()`
[#&#8203;16230](https://redirect.github.com/Automattic/mongoose/issues/16230)
- perf: use kareem 3.3.0
[mongoosejs/kareem#45](https://redirect.github.com/mongoosejs/kareem/issues/45)
[#&#8203;16229](https://redirect.github.com/Automattic/mongoose/issues/16229)
- chore: use TSTyche assertions
[#&#8203;16222](https://redirect.github.com/Automattic/mongoose/issues/16222)
[mrazauskas](https://redirect.github.com/mrazauskas)

</details>

---

### Configuration

📅 **Schedule**: (UTC)

- Branch creation
 - At any time (no schedule defined)
- Automerge
 - At any time (no schedule defined)

🚦 **Automerge**: Disabled by config. Please merge this manually once you
are satisfied.

♻ **Rebasing**: Whenever PR is behind base branch, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR was generated by [Mend Renovate](https://mend.io/renovate/).
View the [repository job
log](https://developer.mend.io/github/antoinezanardi/goat-it-api).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiI0My4xMjMuOCIsInVwZGF0ZWRJblZlciI6IjQzLjEyMy44IiwidGFyZ2V0QnJhbmNoIjoiZGV2ZWxvcCIsImxhYmVscyI6W119-->

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
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

Successfully merging this pull request may close these issues.

3 participants