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

newString zeroes memory contrary to its documentation, significantly slowing it down #22555

Closed
arnetheduck opened this issue Aug 25, 2023 · 8 comments · Fixed by #22572
Closed
Assignees

Comments

@arnetheduck
Copy link
Contributor

Description

newString is supposed to return an uninitialized string, and thus presumably be fast:

## Returns a new string of length `len` but with uninitialized

However, the actual implementations zero the full string leading to significant overhead making the function unfit for its purpose:

https://github.com/nim-lang/Nim/blob/devel/lib/system/strs_v2.nim#L113

Nim Version

1.6, devel, both refc and orc

Current Output

No response

Expected Output

No response

Possible Solution

No response

Additional Information

No response

@ringabout
Copy link
Member

ringabout commented Aug 26, 2023

Isn't it potentially a potential runtime breaking change supposing someone relies on the quirky behaviour? Yeah, let's find out whether it breaks something.

@ringabout
Copy link
Member

It breaks jangko/nimPNG#85

@mratsim
Copy link
Collaborator

mratsim commented Aug 28, 2023

But aren't all Nim data structures supposed to be zero-initialized?

I think the correct least-surprising way forward is to introduce newStringUninit / newStringUninitialized.

@Araq
Copy link
Member

Araq commented Aug 28, 2023

Hmm good point.

@arnetheduck
Copy link
Contributor Author

Came to a similar conclusion while thinking about it over the weekend (newStringUninitialized).

but to complete this section of nim, #19727 really needs addressing as well - the current situation is silly, ie can't newSeqUninitialized a seq[Complex64] nor array[byte, 32] and so on making it impossible to write any sane deserialization code.

@ringabout
Copy link
Member

As you wish, I will add a new function and remove outdated documentation.

@ringabout
Copy link
Member

ringabout commented Aug 28, 2023

ie can't newSeqUninitialized a seq[Complex64] nor array[byte, 32]

Hi, @arnetheduck, supportCopyMem excludes types containing managed memory or having destructors. Is it enough?

@arnetheduck
Copy link
Contributor Author

Hi, @arnetheduck, supportCopyMem excludes types containing managed memory or having destructors. Is it enough?

It's a good enough to start with - I can imagine a few cases where this is too strict as well but it's easy to further relax in the future - ie consider a ref - strictly, if we had the expressivity saying that the instance is not a valid instance, we could allow uninit for ref as well - the situation is similar to what happens with an instance after you moved from it (it's "destroyed" and therefore could be reused without zero-init in theory) but expressing this is tricky - I haven't followed arc closely enough to know for sure where this stands.

Araq pushed a commit that referenced this issue Aug 29, 2023
* fixes newStringUninitialized; implement `newStringUninitialized`

* add a simple test case

* adds a changelog

* Update lib/system.nim

* Apply suggestions from code review

rename to newStringUninit
AmjadHD added a commit to AmjadHD/Nim that referenced this issue Sep 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants