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

drop use of slice header #117

Merged
merged 2 commits into from
Apr 28, 2021
Merged

drop use of slice header #117

merged 2 commits into from
Apr 28, 2021

Conversation

dertseha
Copy link
Contributor

This PR changes how a raw array of bytes is cast back into a slice.
It is in preparation to handle go-gl/gl#132 .

The conversion is now done using a pointer to an arbitrarily large memory block, of which a slice is taken. The size is large enough to cover everything practical, and small enough to still be available on 32bit systems.

I also added another check, to avoid handling an empty slice because of empty strings - which would cause another error.

@errcw
Copy link
Member

errcw commented Apr 26, 2021

The memory allocation makes me somewhat nervous.

Could we do something a little silly and allocate a byte[] version of the string that we then use C.memcpy to copy from?

@dertseha
Copy link
Contributor Author

Such a change would break the current contract. With the current "hard" allocation, the memory where the strings are kept in is stable. A Go-allocated memory area could be moved any time, invalidating any pointer.
This volatile detail is noted for the Str() function, but isn't for Strs().

@dertseha
Copy link
Contributor Author

Addendum: Or did you mean something like first to allocate a continuous byte array for all the strings in Go land, from which then a single memcpy is used into the malloc allocated array?

@errcw
Copy link
Member

errcw commented Apr 27, 2021

I was thinking something along the lines of

offset := 0
for _, str := range strs {
  n := len(str)
  byte[] s := byte[](str) // Unclear whether this is necessary or whether &str[0] has the right data
  C.memcpy(unitptr(data)+offset, unsafe.Pointer(&s[0]), n)
  offset += n
}

@dertseha
Copy link
Contributor Author

Ah, now I understand you - it's not the memory allocation that bothers you, but the way the pointer is cast to Go land.
I will rework it sometime this week.

@errcw
Copy link
Member

errcw commented Apr 28, 2021

I might frame it as the size of the memory allocation is what caused me to think of alternatives; I worry that a big allocation like that might be tricky on a memory-constrained system.

@dertseha
Copy link
Contributor Author

I'm not sure if we're talking about the same thing. Neither my nor your suggested change would modify the C.malloc() call - it's one big chunk either way. That's the only place where allocation happens. (And I don't expect it to be dangerously "big", assuming it's meant for shader sources)

If you are referring to this line: dataSlice := (*[1 << 30]byte)(data)[:n] - no allocation happens here. This line tells Go that data is meant to be a pointer to a fixed byte array of a certain size. And from that array a slice is taken, again ensuring that no unintended out-of-bounds happen. So all "allocation" that is happening here is the meta-data of a fixed array and a slice.
I use this trick in my wrapper for imgui for ages, and have learned it from another wrapper library.

What were you referring to? (I'm happy to add your change as well, I want to be sure we're on the same page)

@errcw
Copy link
Member

errcw commented Apr 28, 2021

Oh oh I understand now, I misread it. Thanks for the detailed explanation! LGTM.

@errcw errcw merged commit 640349a into go-gl:master Apr 28, 2021
@dertseha dertseha deleted the feature/drop-slice-header branch April 28, 2021 20:47
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.

2 participants