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

Make compatible with MSVC #320

Open
wants to merge 3 commits into
base: DF2016
Choose a base branch
from
Open

Conversation

TV4Fun
Copy link

@TV4Fun TV4Fun commented Dec 10, 2016

This fixes compilation with MSVC under windows by removing the VLA from DFInstanceWindows and instead using a unique_ptr to an array on the heap.

This fixes compilation with MSVC under windows by removing the VLA from DFInstanceWindows and instead using a unique_ptr to an array on the heap.
@Hello71
Copy link
Contributor

Hello71 commented Dec 10, 2016

as I said in the other issue, this is bad because you are doing unnecessary heap allocations in a hot path. apparently that code has always checked len > 1024, so just make the buffer 1024 bytes.

@TV4Fun
Copy link
Author

TV4Fun commented Dec 10, 2016

@Hello71, this seems like premature optimization to me. How often is read_string used that an extra heap allocation would actually make that big of a difference? In any case, here is a compromise. We use VLAs when building under GCC, otherwise we use a fixed length of 1024 bytes.

@lethosor
Copy link
Contributor

I would expect it to mainly be used when reading data from DF, maybe 10-100 times per dwarf, so the time it takes to do ~10k allocations probably isn't significant compared to the time it takes to connect to DF and read from it (i.e. I'd expect read_raw to be more of a bottleneck). On the other hand, I could be wrong about how often it's used, and you already have a pretty reasonable maximum length.

@Hello71
Copy link
Contributor

Hello71 commented Dec 10, 2016

I mean just use a 1024-byte buffer because of the length check right before.

@lethosor
Copy link
Contributor

There's also a len < 65536 check after that one, apparently - maybe the second should be removed/changed?

@Hello71
Copy link
Contributor

Hello71 commented Dec 10, 2016

they've been there for over six years, so...

@TV4Fun
Copy link
Author

TV4Fun commented Dec 11, 2016

All of the asserts there are redundant with checks just before them, so I removed them.

@TV4Fun TV4Fun mentioned this pull request Jul 21, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants