feat: Parallel include pointers #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 RestQuery include handling to build and execute a traversal tree that processes independent include paths in parallel; adds a heavy integration test for nested includes; and refactors the benchmarking script and CI logging for include-related performance measurement. Changes
Sequence DiagramsequenceDiagram
actor Client
participant RestQuery
participant ExecTree as ExecutionTree
participant Executor as RecursiveExecutor
participant DB as Database
participant Results
Client->>RestQuery: query with multiple include paths
activate RestQuery
RestQuery->>ExecTree: build execution tree from include paths
RestQuery->>Executor: start recursive execution
activate Executor
rect rgb(240,248,255)
note over Executor: Phase A — fetch root results
Executor->>DB: fetch root objects
DB-->>Executor: root results
end
rect rgb(230,255,230)
note over Executor: Phase B — parallel fetch of independent include paths
par include path 1
Executor->>DB: fetch referenced objects for path 1
DB-->>Executor: results for path 1
and include path 2
Executor->>DB: fetch referenced objects for path 2
DB-->>Executor: results for path 2
end
end
rect rgb(255,250,230)
note over Executor: Phase C — nested paths (depend on parent results)
par nested path A.1
Executor->>DB: fetch nested references
DB-->>Executor: nested results
and nested path A.2
Executor->>DB: fetch nested references
DB-->>Executor: nested results
end
end
Executor->>Results: hydrate root with all included data
Results-->>RestQuery: hydrated results
deactivate Executor
RestQuery-->>Client: return final results
deactivate RestQuery
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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.08% +0.01%
==========================================
Files 187 187
Lines 15224 15234 +10
Branches 177 177
==========================================
+ Hits 14170 14181 +11
+ Misses 1042 1041 -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
| const indexedResults = this.response.results.reduce((indexed, result, i) => { | ||
| indexed[result.objectId] = i; | ||
| return indexed; | ||
| }, {}); | ||
|
|
||
| // Build the execution tree | ||
| const executionTree = {} | ||
| this.include.forEach(path => { | ||
| let current = executionTree; | ||
| path.forEach((node) => { | ||
| if (!current[node]) { | ||
| current[node] = { | ||
| path, | ||
| children: {} | ||
| }; | ||
| } | ||
| current = current[node].children | ||
| }); | ||
| } else if (this.include.length > 0) { | ||
| this.include = this.include.slice(1); | ||
| return this.handleInclude(); | ||
| }); | ||
|
|
||
| const recursiveExecutionTree = async (treeNode) => { | ||
| const { path, children } = treeNode; | ||
| const pathResponse = includePath( | ||
| this.config, | ||
| this.auth, | ||
| this.response, | ||
| path, | ||
| this.context, | ||
| this.restOptions, | ||
| this, | ||
| ); | ||
| if (pathResponse.then) { | ||
| const newResponse = await pathResponse | ||
| newResponse.results.forEach(newObject => { | ||
| // We hydrate the root of each result with sub results | ||
| this.response.results[indexedResults[newObject.objectId]][path[0]] = newObject[path[0]]; | ||
| }) | ||
| } | ||
| return Promise.all(Object.values(children).map(recursiveExecutionTree)); | ||
| } | ||
|
|
||
| return pathResponse; | ||
| await Promise.all(Object.values(executionTree).map(recursiveExecutionTree)); |
There was a problem hiding this comment.
Hydration must not depend solely on objectId.
indexedResults collapses every row whose objectId is missing (e.g. aggregate pipelines that $project out _id while keeping pointer fields) into the single undefined bucket. In that case only the last element in this.response.results is updated and the remaining rows keep their raw pointers, whereas the previous sequential implementation handled these projections correctly. That’s a correctness regression.
Please rework the assignment so we still hydrate by index whenever we cannot resolve an objectId, e.g. by falling back to the loop index.
- newResponse.results.forEach(newObject => {
- this.response.results[indexedResults[newObject.objectId]][path[0]] = newObject[path[0]];
- })
+ newResponse.results.forEach((newObject, idx) => {
+ const targetIndex = indexedResults[newObject.objectId];
+ const target = targetIndex !== undefined ? this.response.results[targetIndex] : this.response.results[idx];
+ if (target) {
+ target[path[0]] = newObject[path[0]];
+ }
+ })🤖 Prompt for AI Agents
In src/RestQuery.js around lines 864 to 905, the hydration currently collapses
rows with missing objectId because indexedResults maps undefined to a single
bucket; update the logic so indexedResults only maps defined objectId values to
their original index, and when applying newResponse.results use the iteration
index as a fallback: in the forEach use the callback index (e.g. (newObject, i))
and compute targetIndex = (newObject.objectId !== undefined &&
indexedResults[newObject.objectId] !== undefined) ?
indexedResults[newObject.objectId] : i, then assign
this.response.results[targetIndex][path[0]] = newObject[path[0]]; ensure you
handle falsy but valid objectId values correctly by explicitly checking
undefined.
There was a problem hiding this comment.
objectId is always present in parse returned include, does the issue still exists ?
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if there are any safeguards preventing includes with aggregate/pipeline queries
rg -n -C3 "pipeline.*include|include.*pipeline" --type=js
# Search for how aggregate queries handle includes
rg -n -C3 "findOptions.pipeline" --type=js -g "src/**"
# Check if there's validation that prevents includes on aggregate queries
rg -n -C3 "aggregate.*include|pipeline.*include" --type=js -g "spec/**"
# Look for any code that explicitly checks for objectId before processing includes
rg -n -C3 "objectId.*include|include.*objectId" --type=js -g "src/RestQuery.js"
Length of output: 717
🏁 Script executed:
#!/bin/bash
# Check the full execute() method to see the order of operations
ast-grep --pattern $'_UnsafeRestQuery.prototype.execute = function ($$$) {
$$$
}'
# Look for any validation that prevents includes with pipeline/aggregate
rg -n -B5 -A10 "handleInclude" src/RestQuery.js | head -60
# Search for tests that combine includes with aggregate queries
rg -n "aggregate.*include|pipeline.*include" spec/ --type=js -C2
# Check if aggregate queries can have the include option at all
rg -n "pipeline.*restOptions|aggregate.*restOptions" --type=js -g "spec/**" | head -20
Length of output: 3354
🏁 Script executed:
#!/bin/bash
# Check how AlwaysSelectedKeys (which includes objectId) is used
rg -n "AlwaysSelectedKeys" src/RestQuery.js -A3 -B3
# Look for aggregate query tests and whether they can have includes
rg -n "aggregate|pipeline" spec/RestQuery.spec.js -C3 | head -100
# Check if there's any code that strips objectId from results
rg -n "delete.*objectId|objectId.*delete" --type=js -g "src/**"
# Look for $project in aggregate pipelines that might exclude _id
rg -n '\$project' spec/ --type=js -C2 | head -50
Length of output: 4131
🏁 Script executed:
#!/bin/bash
# Check if aggregate queries can have include in their options
rg -n "include" spec/ParseQuery.Aggregate.spec.js -C2 | head -50
# Look for the aggregate route handler to see what options it accepts
rg -n "handleFind.*pipeline|pipeline.*handleFind" --type=js -g "src/**"
# Check RestQuery constructor to see if it validates pipeline + include
ast-grep --pattern $'function _UnsafeRestQuery(
$$$
) {
$$$
}'
# Search for any code that prevents includes on pipeline queries
rg -n "pipeline.*include|include.*pipeline" src/ --type=js -C3
Length of output: 9784
@Moumouls, You're right that in normal query results, objectId is always present. However, I found an edge case where it can be missing:
The edge case: When a RestQuery has both:
- A
pipelineoption with$project: { _id: 0 }(explicitly excluding objectId) - An
includeoption
Looking at the code:
- The constructor accepts both
pipeline(line 215) andinclude(line 236) without validation preventing their combination handleInclude()does NOT skip aggregate queries (unlikerunAfterFindTrigger()which checks forpipeline/distinctat line 927)- Tests show aggregate queries can use
$project: { _id: 0 }to exclude objectId (e.g.,spec/ParseQuery.Aggregate.spec.js:362-363)
While this is admittedly a rare/unusual scenario (combining includes with aggregates that exclude _id), the original concern is technically valid.
Recommendation: Either add a guard to skip handleInclude() when pipeline or distinct is present (similar to runAfterFindTrigger), or implement the fallback indexing logic suggested in the original comment to handle this edge case safely.
What do you think? Should we add protection against this edge case or is it acceptable to document that includes aren't supported with aggregate queries?
🧠 Learnings used
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 }.
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.
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()`.
There was a problem hiding this comment.
@mtrezza from you knowledge is it normal to allow _id: 0 on projections ?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
@mtrezza i checked on alpha, currently parse-server doesn't support pipeline + include. so here i'll resolve the coderabbit feedback
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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/ci-performance.yml(2 hunks)benchmark/db-proxy.js(1 hunks)benchmark/performance.js(6 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 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:
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
🧬 Code graph analysis (2)
benchmark/db-proxy.js (1)
benchmark/performance.js (5)
require(14-14)require(15-15)require(34-34)server(148-156)server(444-444)
benchmark/performance.js (1)
benchmark/db-proxy.js (1)
server(15-54)
|
Hi @RahulLanjewar93, we just need to figure out the "project pipeline" support but i believe we will be able to merge this one quickly the target will be PS8 or PS9 |
|
Okay just adding my scenario here, We run parse server in a k8s environment. We have multiple pods running of parse server. To achieve load balancing, we set the direct access to false. Now suppose i do a query which returns 1k objects, i have included a pointer now the requests are sent individually both at db level and server level My idea is to somehow batch the requests made for includer pointer so there is less network round trip. And the dbs calls can be converted to a single db call as well using $in operator on the _is field in mongo Implementation might be difficult or even impossible, but generally this approach should be more performant |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
benchmark/performance.js (1)
37-37: Restore intended 10 ms latency constantThe
LATENCY_MSenvironment variable is set to'10000'(10 seconds), but line 43 and the surrounding context indicate the intent is 10 milliseconds. With 10-second latency and the default iteration count, the include benchmark would take many hours and cause CI timeouts.Apply this diff to fix the latency value:
- env: { ...process.env, PROXY_PORT: '27018', TARGET_PORT: '27017', LATENCY_MS: '10000' }, + env: { ...process.env, PROXY_PORT: '27018', TARGET_PORT: '27017', LATENCY_MS: '10' },
🧹 Nitpick comments (2)
benchmark/performance.js (2)
162-164: Wrapserver.close()in a promise for proper cleanup
server.close()initiates shutdown but completes asynchronously. Without awaiting its callback, the subsequent 500ms timeout may not guarantee complete cleanup, potentially causing port conflicts or resource leaks in subsequent benchmarks.Apply this diff to properly await server closure:
proxyServerCleanup = async () => { - server.close(); - await new Promise(resolve => setTimeout(resolve, 500)); + await new Promise(resolve => server.close(resolve)); proxyServerCleanup = null; };
503-503: Consider increasing cleanup timeout for proxy overheadWith
stopProxy()waiting 500ms and potential additional time for server cleanup, the 1000ms timeout may be close to the minimum required. Increasing to 1500-2000ms would provide a safer margin.Apply this diff if you want additional safety margin:
- setTimeout(() => process.exit(0), 1000); + setTimeout(() => process.exit(0), 1500);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/ci-performance.yml(2 hunks)benchmark/performance.js(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/ci-performance.yml
🧰 Additional context used
🧠 Learnings (6)
📓 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: 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.
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.
📚 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-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-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
🧬 Code graph analysis (1)
benchmark/performance.js (1)
benchmark/db-proxy.js (1)
server(15-54)
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)
benchmark/performance.js(10 hunks)
🧰 Additional context used
🧠 Learnings (9)
📓 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-11-08T13:46:04.917Z
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 0
File: :0-0
Timestamp: 2025-11-08T13:46:04.917Z
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-09-21T15:43:32.265Z
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 9858
File: src/GraphQL/ParseGraphQLServer.js:176-178
Timestamp: 2025-09-21T15:43:32.265Z
Learning: The GraphQL playground feature in ParseGraphQLServer.js (applyPlayground method) is intended for development environments only, which is why it includes the master key in client-side headers.
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
⏰ 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: PostgreSQL 18, PostGIS 3.6
- GitHub Check: PostgreSQL 15, PostGIS 3.4
- GitHub Check: PostgreSQL 16, PostGIS 3.5
- GitHub Check: PostgreSQL 17, PostGIS 3.5
- GitHub Check: MongoDB 6, ReplicaSet
- GitHub Check: PostgreSQL 15, PostGIS 3.5
- GitHub Check: PostgreSQL 15, PostGIS 3.3
- GitHub Check: Node 22
- GitHub Check: Node 18
- GitHub Check: Redis Cache
- GitHub Check: MongoDB 7, ReplicaSet
- GitHub Check: MongoDB 8, ReplicaSet
- GitHub Check: Node 20
- GitHub Check: Docker Build
- GitHub Check: Benchmarks
🔇 Additional comments (3)
benchmark/performance.js (3)
30-32: LGTM: Clean logging abstraction for CI integration.The logging helpers provide a clean abstraction for GitHub Actions structured logging, improving visibility and consistency across the benchmark suite.
102-174: LGTM: Well-structured refactor with improved ergonomics.The options object pattern enhances flexibility and readability. The progress logging and conditional warmup are valuable additions for monitoring long-running benchmarks.
424-433: Verify whether all benchmarks should be enabled.All existing benchmarks are commented out, leaving only the new include benchmark active. Confirm whether this is intentional for testing or if the full suite should be re-enabled before merging.
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 Improvements
Tests
Chores