nim-lang / Nim

Nim is a statically typed compiled systems programming language. It combines successful concepts from mature languages like Python, Ada and Modula. Its design focuses on efficiency, expressiveness, and elegance (in that order of priority).
https://nim-lang.org
Other
16.22k stars 1.46k forks source link

`setLen(0)` allocates memory causing performance regression #23742

Closed arnetheduck closed 1 week ago

arnetheduck commented 1 week ago

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 commented 1 week ago

Which version doesn't allocate memory in refc?

That said, how to measure performance downgradation? Or would you mind checking whether https://github.com/nim-lang/Nim/pull/22690 is the cause

arnetheduck commented 1 week ago

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 commented 1 week ago

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!