perf: Parse.Query.include now fetches pointers at same level in parallel
#9861
Conversation
…moumouls/concurrent-include # Conflicts: # package.json
|
I will reformat the title to use the proper commit message syntax. |
parse-github-assistant
bot
changed the title
|
🚀 Thanks for opening this pull request! |
📝 WalkthroughWalkthroughReworks include processing in Changes
Sequence Diagram(s)sequenceDiagram
participant Q as Query Handler
participant H as handleInclude (async)
participant T as Execution Tree
participant F as Fetchers
participant R as Results
Q->>H: start include processing
H->>T: build traversal tree from include paths
par Parallel independent paths
T->>F: execute path A
T->>F: execute path B
and
F-->>T: return sub-results A
F-->>T: return sub-results B
end
T->>H: all paths resolved
H->>R: hydrate and merge included objects into root results
H->>Q: return final results
rect rgba(200,220,255,0.2)
note right of T: Nested paths wait for parent nodes<br/>then spawn parallel child fetches
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## alpha #9861 +/- ##
==========================================
- Coverage 93.07% 93.07% -0.01%
==========================================
Files 187 187
Lines 15224 15234 +10
Branches 177 177
==========================================
+ Hits 14170 14179 +9
- Misses 1042 1043 +1
Partials 12 12 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…moumouls/concurrent-include # Conflicts: # .gitignore
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
1 similar comment
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/RestQuery.js(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 0
File: :0-0
Timestamp: 2025-11-08T13:46:04.917Z
Learning: When reviewing Parse Server PRs that add new features, always check whether the feature is documented in the README.md file, though for new Parse Server options this is optional rather than required.
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 9445
File: spec/ParseLiveQuery.spec.js:1312-1338
Timestamp: 2025-05-04T20:41:05.147Z
Learning: New tests in the parse-server repository should use async/await with promise-based patterns rather than callback patterns with `done()`.
Learnt from: EmpiDev
Repo: parse-community/parse-server PR: 9770
File: spec/CloudCode.spec.js:446-469
Timestamp: 2025-08-26T14:06:31.853Z
Learning: In the Parse Server codebase, when handling query objects in maybeRunAfterFindTrigger, objects without a where property that contain options like limit/skip should be treated as query JSON with an empty where clause using the spread pattern { where: {}, ...query }, not nested as { where: query }.
📚 Learning: 2025-08-26T14:06:31.853Z
Learnt from: EmpiDev
Repo: parse-community/parse-server PR: 9770
File: spec/CloudCode.spec.js:446-469
Timestamp: 2025-08-26T14:06:31.853Z
Learning: In the Parse Server codebase, when handling query objects in maybeRunAfterFindTrigger, objects without a where property that contain options like limit/skip should be treated as query JSON with an empty where clause using the spread pattern { where: {}, ...query }, not nested as { where: query }.
Applied to files:
src/RestQuery.js
Moumouls
changed the title
|
So @mtrezza depending of DB co location with the server or even DB performance ( under load, slow queries), this PR introduce a important optimization in case of 10ms DB latency it's already a 23x time faster 🚀 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
spec/RestQuery.spec.js (2)
390-390: Consider a more descriptive test name.The term "battle test" is informal. A clearer name would better describe the test's purpose and expected behavior.
Consider renaming to something like:
- it('battle test parallel include with 100 nested includes', async () => { + it('should efficiently load 100 nested includes in parallel', async () => {
445-447: Remove debug timing statements.The
console.timeandconsole.timeEndcalls are debug statements that clutter test output. If performance validation is needed, capture the timing in a variable and assert against a threshold; otherwise, remove these statements.Apply this diff to remove the timing statements:
- console.time('query.find'); const results = await query.find(); - console.timeEnd('query.find');Alternatively, if you want to validate performance:
+ const startTime = Date.now(); const results = await query.find(); + const duration = Date.now() - startTime; + expect(duration).toBeLessThan(5000); // Adjust threshold as needed - console.time('query.find'); - console.timeEnd('query.find');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
spec/RestQuery.spec.js(1 hunks)
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 0
File: :0-0
Timestamp: 2025-11-08T13:46:04.917Z
Learning: When reviewing Parse Server PRs that add new features, always check whether the feature is documented in the README.md file, though for new Parse Server options this is optional rather than required.
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 9445
File: spec/ParseLiveQuery.spec.js:1312-1338
Timestamp: 2025-05-04T20:41:05.147Z
Learning: New tests in the parse-server repository should use async/await with promise-based patterns rather than callback patterns with `done()`.
📚 Learning: 2025-04-30T19:31:35.344Z
Learnt from: RahulLanjewar93
Repo: parse-community/parse-server PR: 9744
File: spec/ParseLiveQuery.spec.js:0-0
Timestamp: 2025-04-30T19:31:35.344Z
Learning: In the Parse Server codebase, the functions in QueryTools.js are typically tested through end-to-end behavior tests rather than direct unit tests, even though the functions are exported from the module.
Applied to files:
spec/RestQuery.spec.js
📚 Learning: 2025-05-04T20:41:05.147Z
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 9445
File: spec/ParseLiveQuery.spec.js:1312-1338
Timestamp: 2025-05-04T20:41:05.147Z
Learning: New tests in the parse-server repository should use async/await with promise-based patterns rather than callback patterns with `done()`.
Applied to files:
spec/RestQuery.spec.js
📚 Learning: 2025-08-27T12:33:06.237Z
Learnt from: EmpiDev
Repo: parse-community/parse-server PR: 9770
File: src/triggers.js:467-477
Timestamp: 2025-08-27T12:33:06.237Z
Learning: In the Parse Server codebase, maybeRunAfterFindTrigger is called in production with Parse.Query objects constructed via withJSON(), so the plain object query handling bug only affects tests, not production code paths.
Applied to files:
spec/RestQuery.spec.js
📚 Learning: 2025-05-09T09:59:06.289Z
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 9445
File: spec/ParseLiveQuery.spec.js:1340-1375
Timestamp: 2025-05-09T09:59:06.289Z
Learning: New tests in the parse-server repository should use async/await with promise-based patterns rather than callback patterns with `done()`. The preferred pattern is to create a Promise that resolves when an expected event occurs, then await that Promise.
Applied to files:
spec/RestQuery.spec.js
📚 Learning: 2025-05-09T09:59:06.289Z
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 9445
File: spec/ParseLiveQuery.spec.js:1340-1375
Timestamp: 2025-05-09T09:59:06.289Z
Learning: Tests in the parse-server repository should use promise-based approaches rather than callback patterns with `done()`. Use a pattern where a Promise is created that resolves when the event occurs, then await that promise.
Applied to files:
spec/RestQuery.spec.js
📚 Learning: 2025-10-16T19:27:05.311Z
Learnt from: Moumouls
Repo: parse-community/parse-server PR: 9883
File: spec/CloudCodeLogger.spec.js:410-412
Timestamp: 2025-10-16T19:27:05.311Z
Learning: In spec/CloudCodeLogger.spec.js, the test "should log cloud function triggers using the silent log level" (around lines 383-420) is known to be flaky and requires the extra `await new Promise(resolve => setTimeout(resolve, 100))` timeout after awaiting `afterSavePromise` for reliability, even though it may appear redundant.
Applied to files:
spec/RestQuery.spec.js
📚 Learning: 2025-08-26T14:06:31.853Z
Learnt from: EmpiDev
Repo: parse-community/parse-server PR: 9770
File: spec/CloudCode.spec.js:446-469
Timestamp: 2025-08-26T14:06:31.853Z
Learning: In the Parse Server codebase, when handling query objects in maybeRunAfterFindTrigger, objects without a where property that contain options like limit/skip should be treated as query JSON with an empty where clause using the spread pattern { where: {}, ...query }, not nested as { where: query }.
Applied to files:
spec/RestQuery.spec.js
🧬 Code graph analysis (1)
spec/RestQuery.spec.js (1)
spec/helper.js (2)
Parse(4-4)i(346-346)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: PostgreSQL 15, PostGIS 3.5
- GitHub Check: PostgreSQL 16, PostGIS 3.5
- GitHub Check: PostgreSQL 17, PostGIS 3.5
- GitHub Check: PostgreSQL 15, PostGIS 3.4
- GitHub Check: PostgreSQL 18, PostGIS 3.6
- GitHub Check: PostgreSQL 15, PostGIS 3.3
🔇 Additional comments (1)
spec/RestQuery.spec.js (1)
390-470: Excellent test coverage for parallel include functionality.This test comprehensively validates the parallel include feature by:
- Creating a realistic 3-level object hierarchy (1 root → 10 Level1 → 100 Level2)
- Testing 110 nested include paths simultaneously
- Verifying all nested objects are properly hydrated
- Using async/await patterns consistently (per Parse Server conventions)
The test directly addresses the PR objective to validate parallel include loading with deep nesting. The structure is clear and assertions are thorough.
Based on learnings
|
I've added a perf check in the CI. It runs only specific tests though, see docs. Do you want to add a benchmark to see the diff? Note that this is alpha, not sure whether it works reliably. I tested it with another PR and it showed no perf impact. However, it shows a perf regression issue for this PR here for creating objects. The other PR shows similar values for the baseline, so the baseline itself may be correct. It could be that the perf test algorithm has an issue (miscalculating an improvement for a regression). This PR only seems to change the |
It will not show much @mtrezza as you can see on my benchmark
|
|
Interested in this pr, Any eta on this? When can it come to v7/v8 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
benchmark/performance.js (1)
296-340: Suggest minor consistency and clarity improvements.The benchmark implementation is functionally correct. A few optional refinements:
Naming inconsistency: Line 297 says "Parallel Include Pointers" but line 335 uses "Query with Include (2 levels)". Consider using consistent terminology.
Redundant modulo operations: Lines 320 and 330 use
i % level2Objects.lengthandi % level1Objects.lengthwhereiranges from 0-9 and both arrays have length 10, making the modulo redundant.Query variation: Unlike
benchmarkSimpleQuery(which varies the query using a counter), all iterations here query the same data. Consider adding variation if you want to measure cache effects or query diversity.Example diff for consistency:
/** - * Benchmark: Query with Include (Parallel Include Pointers) + * Benchmark: Query with Include (2 levels) */Example diff for redundant modulo (lines 320, 330):
- obj.set('level2', level2Objects[i % level2Objects.length]); + obj.set('level2', level2Objects[i]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
benchmark/performance.js(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 0
File: :0-0
Timestamp: 2025-11-08T13:46:04.917Z
Learning: When reviewing Parse Server PRs that add new features, always check whether the feature is documented in the README.md file, though for new Parse Server options this is optional rather than required.
📚 Learning: 2025-04-30T19:31:35.344Z
Learnt from: RahulLanjewar93
Repo: parse-community/parse-server PR: 9744
File: spec/ParseLiveQuery.spec.js:0-0
Timestamp: 2025-04-30T19:31:35.344Z
Learning: In the Parse Server codebase, the functions in QueryTools.js are typically tested through end-to-end behavior tests rather than direct unit tests, even though the functions are exported from the module.
Applied to files:
benchmark/performance.js
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Code Analysis (javascript)
🔇 Additional comments (1)
benchmark/performance.js (1)
390-392: No duplicates found—benchmark code is correct.Verification confirms only one
benchmarkQueryWithIncludefunction definition and one invocation exist. The second regex match at line 299 is the function definition itself, not a duplicate. The invocation at line 392 follows the established benchmark pattern correctly.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
benchmark/performance.js (1)
370-419: Critical: Object creation still inside measured operation - past fix not applied.The past review comment (marked as "Addressed in commits 713a4d0 to 35f1809") flagged this exact issue, but the problem persists in the current code. Lines 384-412 create 30 objects (10×Level2 + 10×Level1 + 10×Root) inside the
operationfunction, so each iteration accumulates 30,000 objects with 1000 iterations, causing:
- Progressive query slowdown as the dataset grows
- Unreliable measurements (different dataset size per iteration)
- Memory and storage issues
- Skewed benchmark results that don't reflect the intended use case
The object setup must be moved outside
measureOperation, following the pattern inbenchmarkObjectReadandbenchmarkObjectUpdate. Only the query with include should be measured.Apply this refactor to move setup outside the measured operation:
async function benchmarkQueryWithInclude() { // Setup: Create nested object hierarchy const Level2Class = Parse.Object.extend('Level2'); const Level1Class = Parse.Object.extend('Level1'); const RootClass = Parse.Object.extend('Root'); + // Create 10 Level2 objects + const level2Objects = []; + for (let i = 0; i < 10; i++) { + const obj = new Level2Class(); + obj.set('name', `level2-${i}`); + obj.set('value', i); + level2Objects.push(obj); + } + await Parse.Object.saveAll(level2Objects); + + // Create 10 Level1 objects, each pointing to a Level2 object + const level1Objects = []; + for (let i = 0; i < 10; i++) { + const obj = new Level1Class(); + obj.set('name', `level1-${i}`); + obj.set('level2', level2Objects[i % level2Objects.length]); + level1Objects.push(obj); + } + await Parse.Object.saveAll(level1Objects); + + // Create 10 Root objects, each pointing to a Level1 object + const rootObjects = []; + for (let i = 0; i < 10; i++) { + const obj = new RootClass(); + obj.set('name', `root-${i}`); + obj.set('level1', level1Objects[i % level1Objects.length]); + rootObjects.push(obj); + } + await Parse.Object.saveAll(rootObjects); + return measureOperation({ name: 'Query with Include (2 levels)', - skipWarmup: true, dbLatency: 100, operation: async () => { - // Create 10 Level2 objects - const level2Objects = []; - for (let i = 0; i < 10; i++) { - const obj = new Level2Class(); - obj.set('name', `level2-${i}`); - obj.set('value', i); - level2Objects.push(obj); - } - await Parse.Object.saveAll(level2Objects); - - // Create 10 Level1 objects, each pointing to a Level2 object - const level1Objects = []; - for (let i = 0; i < 10; i++) { - const obj = new Level1Class(); - obj.set('name', `level1-${i}`); - obj.set('level2', level2Objects[i % level2Objects.length]); - level1Objects.push(obj); - } - await Parse.Object.saveAll(level1Objects); - - // Create 10 Root objects, each pointing to a Level1 object - const rootObjects = []; - for (let i = 0; i < 10; i++) { - const obj = new RootClass(); - obj.set('name', `root-${i}`); - obj.set('level1', level1Objects[i % level1Objects.length]); - rootObjects.push(obj); - } - await Parse.Object.saveAll(rootObjects); - const query = new Parse.Query('Root'); query.include('level1.level2'); await query.find(); }, }); }
🧹 Nitpick comments (3)
benchmark/MongoLatencyWrapper.js (2)
20-23: Consider scope and lifecycle of originalMethods Map.The module-scoped
originalMethodsMap could cause issues ifwrapMongoDBWithLatencyis called multiple times without proper unwrapping between calls. While the current usage pattern inperformance.jswraps and unwraps per benchmark, the wrapper doesn't guard against:
- Calling
wrapMongoDBWithLatencytwice without unwrapping (would lose the true originals)- Race conditions if multiple tests run concurrently
Consider adding a guard to prevent double-wrapping:
function wrapMongoDBWithLatency(latencyMs) { + if (originalMethods.size > 0) { + throw new Error('MongoDB is already wrapped. Call unwrap() before wrapping again.'); + } + if (typeof latencyMs !== 'number' || latencyMs < 0) { throw new Error('latencyMs must be a non-negative number'); }
65-71: Remove or clarify the synchronous fallback path (lines 65-71) as unreachable dead code.MongoDB Collection methods only return Promises or Cursors. All methods in
methodsToWrap(lines 94-116)—includingfind(),insertOne(),updateOne(), etc.—return either a FindCursor (handled at lines 43-54) or a Promise (handled at lines 57-63). The synchronous fallback (lines 65-71) will never execute. Either remove it or add a comment explaining it's a defensive safeguard that should never be reached.benchmark/performance.js (1)
25-25: Consider making LOG_ITERATIONS configurable via environment variable.While the current hard-coded
falsevalue is reasonable for typical use, allowing control via environment variable would provide flexibility for debugging without code changes:-const LOG_ITERATIONS = false; +const LOG_ITERATIONS = process.env.LOG_ITERATIONS === 'true';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
benchmark/MongoLatencyWrapper.js(1 hunks)benchmark/performance.js(9 hunks)
🧰 Additional context used
🧠 Learnings (10)
📓 Common learnings
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 0
File: :0-0
Timestamp: 2025-11-08T13:46:04.940Z
Learning: When reviewing Parse Server PRs that add new features, always check whether the feature is documented in the README.md file, though for new Parse Server options this is optional rather than required.
📚 Learning: 2025-11-08T13:46:04.940Z
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 0
File: :0-0
Timestamp: 2025-11-08T13:46:04.940Z
Learning: For new Parse Server options, verify that the option is documented in src/Options/index.js and that npm run definitions has been executed to reflect changes in src/Options/docs.js and src/Options/Definitions.js. README.md documentation is a bonus but not required for new options.
Applied to files:
benchmark/performance.js
📚 Learning: 2025-04-30T19:31:35.344Z
Learnt from: RahulLanjewar93
Repo: parse-community/parse-server PR: 9744
File: spec/ParseLiveQuery.spec.js:0-0
Timestamp: 2025-04-30T19:31:35.344Z
Learning: In the Parse Server codebase, the functions in QueryTools.js are typically tested through end-to-end behavior tests rather than direct unit tests, even though the functions are exported from the module.
Applied to files:
benchmark/performance.js
📚 Learning: 2025-08-27T09:08:34.252Z
Learnt from: EmpiDev
Repo: parse-community/parse-server PR: 9770
File: src/triggers.js:446-454
Timestamp: 2025-08-27T09:08:34.252Z
Learning: When analyzing function signature changes in Parse Server codebase, verify that call sites are actually incorrect before flagging them. Passing tests are a strong indicator that function calls are already properly aligned with new signatures.
Applied to files:
benchmark/performance.js
📚 Learning: 2025-05-04T20:41:05.147Z
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 9445
File: spec/ParseLiveQuery.spec.js:1312-1338
Timestamp: 2025-05-04T20:41:05.147Z
Learning: New tests in the parse-server repository should use async/await with promise-based patterns rather than callback patterns with `done()`.
Applied to files:
benchmark/performance.js
📚 Learning: 2025-10-16T19:27:05.311Z
Learnt from: Moumouls
Repo: parse-community/parse-server PR: 9883
File: spec/CloudCodeLogger.spec.js:410-412
Timestamp: 2025-10-16T19:27:05.311Z
Learning: In spec/CloudCodeLogger.spec.js, the test "should log cloud function triggers using the silent log level" (around lines 383-420) is known to be flaky and requires the extra `await new Promise(resolve => setTimeout(resolve, 100))` timeout after awaiting `afterSavePromise` for reliability, even though it may appear redundant.
Applied to files:
benchmark/performance.js
📚 Learning: 2025-08-26T14:06:31.853Z
Learnt from: EmpiDev
Repo: parse-community/parse-server PR: 9770
File: spec/CloudCode.spec.js:446-469
Timestamp: 2025-08-26T14:06:31.853Z
Learning: In the Parse Server codebase, when handling query objects in maybeRunAfterFindTrigger, objects without a where property that contain options like limit/skip should be treated as query JSON with an empty where clause using the spread pattern { where: {}, ...query }, not nested as { where: query }.
Applied to files:
benchmark/performance.js
📚 Learning: 2025-08-27T12:33:06.237Z
Learnt from: EmpiDev
Repo: parse-community/parse-server PR: 9770
File: src/triggers.js:467-477
Timestamp: 2025-08-27T12:33:06.237Z
Learning: In the Parse Server codebase, maybeRunAfterFindTrigger is called in production with Parse.Query objects constructed via withJSON(), so the plain object query handling bug only affects tests, not production code paths.
Applied to files:
benchmark/performance.js
📚 Learning: 2025-05-09T09:59:06.289Z
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 9445
File: spec/ParseLiveQuery.spec.js:1340-1375
Timestamp: 2025-05-09T09:59:06.289Z
Learning: New tests in the parse-server repository should use async/await with promise-based patterns rather than callback patterns with `done()`. The preferred pattern is to create a Promise that resolves when an expected event occurs, then await that Promise.
Applied to files:
benchmark/performance.js
📚 Learning: 2025-05-09T09:59:06.289Z
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 9445
File: spec/ParseLiveQuery.spec.js:1340-1375
Timestamp: 2025-05-09T09:59:06.289Z
Learning: Tests in the parse-server repository should use promise-based approaches rather than callback patterns with `done()`. Use a pattern where a Promise is created that resolves when the event occurs, then await that promise.
Applied to files:
benchmark/performance.js
🧬 Code graph analysis (2)
benchmark/performance.js (1)
benchmark/MongoLatencyWrapper.js (2)
require(20-20)result(40-40)
benchmark/MongoLatencyWrapper.js (2)
benchmark/performance.js (3)
require(15-15)require(16-16)require(17-17)src/Adapters/Storage/Mongo/MongoCollection.js (1)
Collection(2-2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (15)
- GitHub Check: Node 22
- GitHub Check: PostgreSQL 15, PostGIS 3.3
- GitHub Check: PostgreSQL 18, PostGIS 3.6
- GitHub Check: Node 20
- GitHub Check: PostgreSQL 15, PostGIS 3.4
- GitHub Check: PostgreSQL 15, PostGIS 3.5
- GitHub Check: PostgreSQL 17, PostGIS 3.5
- GitHub Check: Node 18
- GitHub Check: PostgreSQL 16, PostGIS 3.5
- GitHub Check: MongoDB 7, ReplicaSet
- GitHub Check: MongoDB 6, ReplicaSet
- GitHub Check: MongoDB 8, ReplicaSet
- GitHub Check: Redis Cache
- GitHub Check: Docker Build
- GitHub Check: Benchmarks
🔇 Additional comments (7)
benchmark/MongoLatencyWrapper.js (2)
79-133: LGTM - Well-structured latency wrapper with proper cleanup.The implementation correctly:
- Validates input parameters
- Optimizes the zero-latency case
- Covers a comprehensive set of MongoDB Collection methods
- Provides proper cleanup via the unwrap function
The unwrap pattern ensures methods are restored and the Map is cleared, preventing state leakage between benchmark runs.
135-137: LGTM - Clean module interface.Exports only the public API, keeping internal implementation details (
wrapMethod,originalMethods) private.benchmark/performance.js (5)
13-13: LGTM - Appropriate use of GitHub Actions logging.The
@actions/corelibrary provides structured logging for CI environments and integrates well with the benchmark output format. The logging helpers provide a clean abstraction layer.Also applies to: 17-17, 32-33
99-101: LGTM - Useful utility for benchmark isolation.Resetting the Parse SDK URL between benchmarks ensures each benchmark starts from a clean state, preventing state leakage.
112-191: LGTM - Well-designed measurement infrastructure.The refactored
measureOperationprovides:
- Clean options-based API
- Per-benchmark latency control with proper cleanup in finally block
- Detailed progress logging for CI visibility
- Sound statistical analysis with IQR outlier filtering
The integration with
wrapMongoDBWithLatencycorrectly wraps before measurement and unwraps in the finally block, ensuring cleanup even on errors.
196-368: LGTM - Consistent migration to new API.All existing benchmarks correctly migrated to the new
measureOperationoptions-based API while preserving their original logic.
441-458: LGTM - Improved benchmark orchestration.The data-driven approach with the benchmarks array provides:
- Clear declaration of all benchmarks upfront
- Consistent isolation (reset + cleanup) before each benchmark
- Easier maintenance when adding/removing benchmarks
Signed-off-by: Manuel <[email protected]>
|
@Moumouls I was able to measure the performance improvement of this PR by introducing artificial latency to DB requests in the benchmark, see summary. And it behaves as you said, the higher the latency, the higher the improvement. 
The performance improvement applies to parallel includes (see benchmark code |
|
@mtrezza nested pointers are improved but in a specific way, the parallelization happen at each "nested level", so in case of p1.p2.p3 p3, p4, p5 are actually loaded in parallel because after p2 we know each objectId As mentioned in the description: parallelization potential happen at each level, because parse-server need to discover objectId before performing include calls. And currently i don't see an approach where you can get p1.p2.p3, without knowing p2 and p3 pointer ref. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
benchmark/performance.js (1)
445-528: LGTM: Nested include benchmark correctly tests parallel leaf-node fetching.The benchmark properly tests the PR's optimization for parallel fetching at each nested level. The setup correctly creates the object hierarchy outside the measured operation, and the include paths
['level1.level2.level3A', 'level1.level2.level3B', 'level1.level2.level3C']will effectively demonstrate that after resolvinglevel2, the three leaf pointers can be fetched in parallel.Optional: Consider removing
skipWarmup: truefor measurement consistency.Both new include benchmarks skip the warmup phase. While this reduces test duration (saving ~20 iterations at 100ms latency), including warmup would provide more reliable measurements by allowing JIT optimization and cache warming. For consistency with other benchmarks and more stable metrics, consider removing
skipWarmup: trueunless test duration is critical.Apply this diff to both benchmarks if desired:
return measureOperation({ name, - skipWarmup: true, dbLatency: 100, iterations: 100,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
benchmark/performance.js(13 hunks)
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 0
File: :0-0
Timestamp: 2025-11-08T13:46:04.940Z
Learning: When reviewing Parse Server PRs that add new features, always check whether the feature is documented in the README.md file, though for new Parse Server options this is optional rather than required.
📚 Learning: 2025-10-16T19:27:05.311Z
Learnt from: Moumouls
Repo: parse-community/parse-server PR: 9883
File: spec/CloudCodeLogger.spec.js:410-412
Timestamp: 2025-10-16T19:27:05.311Z
Learning: In spec/CloudCodeLogger.spec.js, the test "should log cloud function triggers using the silent log level" (around lines 383-420) is known to be flaky and requires the extra `await new Promise(resolve => setTimeout(resolve, 100))` timeout after awaiting `afterSavePromise` for reliability, even though it may appear redundant.
Applied to files:
benchmark/performance.js
📚 Learning: 2025-08-26T14:06:31.853Z
Learnt from: EmpiDev
Repo: parse-community/parse-server PR: 9770
File: spec/CloudCode.spec.js:446-469
Timestamp: 2025-08-26T14:06:31.853Z
Learning: In the Parse Server codebase, when handling query objects in maybeRunAfterFindTrigger, objects without a where property that contain options like limit/skip should be treated as query JSON with an empty where clause using the spread pattern { where: {}, ...query }, not nested as { where: query }.
Applied to files:
benchmark/performance.js
📚 Learning: 2025-08-27T12:33:06.237Z
Learnt from: EmpiDev
Repo: parse-community/parse-server PR: 9770
File: src/triggers.js:467-477
Timestamp: 2025-08-27T12:33:06.237Z
Learning: In the Parse Server codebase, maybeRunAfterFindTrigger is called in production with Parse.Query objects constructed via withJSON(), so the plain object query handling bug only affects tests, not production code paths.
Applied to files:
benchmark/performance.js
📚 Learning: 2025-04-30T19:31:35.344Z
Learnt from: RahulLanjewar93
Repo: parse-community/parse-server PR: 9744
File: spec/ParseLiveQuery.spec.js:0-0
Timestamp: 2025-04-30T19:31:35.344Z
Learning: In the Parse Server codebase, the functions in QueryTools.js are typically tested through end-to-end behavior tests rather than direct unit tests, even though the functions are exported from the module.
Applied to files:
benchmark/performance.js
📚 Learning: 2025-05-09T09:59:06.289Z
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 9445
File: spec/ParseLiveQuery.spec.js:1340-1375
Timestamp: 2025-05-09T09:59:06.289Z
Learning: New tests in the parse-server repository should use async/await with promise-based patterns rather than callback patterns with `done()`. The preferred pattern is to create a Promise that resolves when an expected event occurs, then await that Promise.
Applied to files:
benchmark/performance.js
📚 Learning: 2025-05-09T09:59:06.289Z
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 9445
File: spec/ParseLiveQuery.spec.js:1340-1375
Timestamp: 2025-05-09T09:59:06.289Z
Learning: Tests in the parse-server repository should use promise-based approaches rather than callback patterns with `done()`. Use a pattern where a Promise is created that resolves when the event occurs, then await that promise.
Applied to files:
benchmark/performance.js
📚 Learning: 2025-05-04T20:41:05.147Z
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 9445
File: spec/ParseLiveQuery.spec.js:1312-1338
Timestamp: 2025-05-04T20:41:05.147Z
Learning: New tests in the parse-server repository should use async/await with promise-based patterns rather than callback patterns with `done()`.
Applied to files:
benchmark/performance.js
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (16)
- GitHub Check: PostgreSQL 18, PostGIS 3.6
- GitHub Check: PostgreSQL 16, PostGIS 3.5
- GitHub Check: PostgreSQL 17, PostGIS 3.5
- GitHub Check: PostgreSQL 15, PostGIS 3.4
- GitHub Check: Node 20
- GitHub Check: Node 22
- GitHub Check: PostgreSQL 15, PostGIS 3.5
- GitHub Check: PostgreSQL 15, PostGIS 3.3
- GitHub Check: Node 18
- GitHub Check: Redis Cache
- GitHub Check: MongoDB 7, ReplicaSet
- GitHub Check: MongoDB 8, ReplicaSet
- GitHub Check: MongoDB 6, ReplicaSet
- GitHub Check: Code Analysis (javascript)
- GitHub Check: Docker Build
- GitHub Check: Benchmarks
🔇 Additional comments (2)
benchmark/performance.js (2)
203-207: LGTM: Clean parameterization of benchmark functions.The refactoring to accept a
nameparameter across all benchmark functions and pass it through from the benchmark definitions is well-structured and enables clearer result reporting.Also applies to: 223-239, 251-268, 282-299, 312-316, 337-341, 357-374, 549-567
384-443: LGTM: Parallel include benchmark correctly structured.The benchmark properly tests parallel pointer fetching at the same level. Object creation is correctly placed outside the measured operation (addressing the previous review concern), and the include syntax
['pointerA', 'pointerB', 'pointerC']will effectively demonstrate the PR's parallel fetching improvements.
|
Thanks for clarifying, I modified to test for nested pointers and it shows an improvement there now as well - great job! This PR was also a good occasion to add the performance CI job, so it's a double win. 
I may be possible if we run a query in an aggregation pipeline to determine the object ID of p3 completely on the MongoDB side. That would mean only 1 roundtrip instead of n (= path depth) between server and DB. Just a thought, outside of the scope of this PR anyway. |
mtrezza
changed the title
Parse.Query.include now fetches pointers in parallel instead of in serial
mtrezza
changed the title
Parse.Query.include now fetches pointers in parallel instead of in serialParse.Query.include now fetches pointers at same level in parallel
# [8.5.0-alpha.8](8.5.0-alpha.7...8.5.0-alpha.8) (2025-11-17) ### Performance Improvements * `Parse.Query.include` now fetches pointers at same level in parallel ([#9861](#9861)) ([dafea21](dafea21))
|
🎉 This change has been released in version 8.5.0-alpha.8 |
I think it's the approach but it think it could be messy and i'm not sure its possible because pointers sill run many hooks/acl and more |
True that |
4 participants
Pull Request
Issue
Related to : #6501
Closes: #7981
Approach
I try to avoid tricks and weird pattern as much as possible.
The approach is "Plan and execute", so we build a tree, and then we execute using parallel promises, and we hydrate the tree using some references
Each branch is recursively executed independently as fast as possible, and new parallelism is introduced at each child node.
Results
Benchmark procedure: Create a Parse.Object with 10 points on it, then each child have also 10 points, resulting in 100 includes
Before optimization:
After optimization
Tasks
Summary by CodeRabbit
Performance
Tests
Benchmark
Chores