Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

setLen(0) allocates memory causing performance regression #23742

Closed
arnetheduck opened this issue Jun 19, 2024 · 3 comments · Fixed by #23745
Closed

setLen(0) allocates memory causing performance regression #23742

arnetheduck opened this issue Jun 19, 2024 · 3 comments · Fixed by #23745
Assignees

Comments

@arnetheduck
Copy link
Contributor

Description

https://github.com/status-im/Nim/blob/v2.0.6/lib/system/sysstr.nim#L223

var vvvv: string
vvvv.setLen(0) # causes allocation

this is a regression from earlier - setLen should obvious not allocate anything in this case. mm:orc is ok.

Nim Version

2.0.6/refc

Current Output

No response

Expected Output

No response

Possible Solution

No response

Additional Information

No response

@ringabout
Copy link
Member

ringabout commented Jun 20, 2024

Which version doesn't allocate memory in refc?

That said, how to measure performance downgradation? Or would you mind checking whether #22690 is the cause

@arnetheduck
Copy link
Contributor Author

That said, how to measure performance downgradation?

This is on upgrade from 1.6 to 2.0.

In this case it was found by backtracing where allocations are coming from, in a virtual machine, and seeing this line turn up which didn't before..

that said, what could be happening is that in 1.6, the allocation was happening somewhere else before and now doesn't, causing the accounting to change from another line to here, ie maybe before the setLen was a noop because it was operating on an already-allocated seq.

ie this might not be a setLen regression per se, but rather part of a broader change in behavior.

Anyway, allocating in setLen(0) doesn't make sense - the reason to use setLen like this is to reuse existing memory if it's allocated already but if it's not, it shouldn't be - same for newSeq(0), @[] and so on (which may happen deep in generic/parametrized code and shouldn't do anything).

#22690 is a good change in general, ie it's causing improvements overall, anecdotally at least (we're still testing).

@Araq
Copy link
Member

Araq commented Jun 20, 2024

Anyway, allocating in setLen(0) doesn't make sense - the reason to use setLen like this is to reuse existing memory if it's allocated already but if it's not, it shouldn't be

Correct!

@ringabout ringabout self-assigned this Jun 20, 2024
@Araq Araq closed this as completed in 2bef087 Jun 21, 2024
narimiran pushed a commit that referenced this issue Jun 24, 2024
…strs/seqs for refc (#23745)

fixes #23742

Before my PR, `setLen(0)` doesn't free buffer if `s != nil`, but it
allocated unnecessary memory for `strs`. This PR rectifies this
behavior. `setLen(0)` no longer allocates memory for uninitialized
strs/seqs

(cherry picked from commit 2bef087)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants