Conversation
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…y and consistency
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -633,9 +656,13 @@ proc fillSeqOp(c: var TLiftCtx; t: PType; body, x, y: PNode) = | |||
| # var i = 0 | |||
| # while i < y.len: dest[i] = y[i]; inc(i) | |||
| # This is usually more efficient than a destroy/create pair. | |||
| # For trivially copyable types, use bulk copyMem instead of element loop. | |||
| checkSelfAssignment(c, t, body, x, y) | |||
| body.add setLenSeqCall(c, t, x, y) | |||
| forallElements(c, t, body, x, y) | |||
| if supportsCopyMem(t.elementType): | |||
| genBulkCopySeq(c, t, body, x, y) | |||
| else: | |||
| forallElements(c, t, body, x, y) | |||
There was a problem hiding this comment.
New bulk-copy path for seq assignment/copy is performance-critical and changes generated code for ORC/ARC. Please add a regression test that asserts this path is exercised for a trivially-copyable element type (e.g., seq[byte]), ideally by checking --expandArc/--expandOrc (or other compiler output) contains a call to nimCopySeqPayload, so the optimization doesn’t silently regress again.
| cond.typ = getSysType(c.g, c.info, tyBool) | ||
| body.add genIf(c, cond, newTreeI(nkReturnStmt, c.info, newNodeI(nkEmpty, c.info))) | ||
|
|
||
| proc genBulkCopySeq(c: var TLiftCtx; t: PType; body, x, y: PNode) = |
There was a problem hiding this comment.
why generate this in the compiler? can't it be library code?
There was a problem hiding this comment.
The =copy hook for seq is still special magic in Nim 2. Mostly caused by the fact that people expect seq to work within Nim's VM which doesn't support the required cast/alloc/etc stuff. There is also const s = @["abc"] which is valid Nim code (makes no sense here but comes up for Table in const sections).
fixes #25687
This pull request introduces an optimization for sequence (
seq) assignments and copies in the Nim compiler, enabling bulk memory copying for sequences whose element types are trivially copyable (i.e., no GC references or destructors). This can significantly improve performance for such types by avoiding per-element loops.Key changes:
Compiler code generation improvements
elemSupportsCopyMemfunction incompiler/liftdestructors.nimto detect if a sequence's element type is trivially copyable (no GC refs, no destructors).fillSeqOpprocedure to use a newgenBulkCopySeqcode path for eligible element types, generating a call tonimCopySeqPayloadfor efficient bulk copying. Fallback to the element-wise loop remains for non-trivial types. [1] [2]Runtime support
nimCopySeqPayloadprocedure inlib/system/seqs_v2.nim, which performs the actual bulk memory copy of sequence data usingcopyMem. This is only used for types that are safe for such an operation.These changes collectively improve the efficiency of sequence operations for simple types, while maintaining correctness for complex types.
Benchmarked the original micro-benchmark:
refc: 3.52s user 0.02s system 99% cpu 3.538 total
orc (after change): 3.46s user 0.01s system 99% cpu 3.476 total