goby-lang / goby

Goby - Yet another programming language written in Go
MIT License
3.49k stars 171 forks source link

Hash accessing is not thread-safe #469

Closed st0012 closed 6 years ago

st0012 commented 6 years ago

Conclusion: We shouldn't use Go's sync.Map to implement normal hash, because it's performance it's poor in normal condition. Reference

st0012 commented 6 years ago

@saveriomiroddi @ear7h Will you want to try this?

ear7h commented 6 years ago

I can do this. Seems like just mutexes on read and write operations within the builtin methods.

st0012 commented 6 years ago

@ear7h We shouldn't do that until we have no choice. That will slow down the whole program.

ear7h commented 6 years ago

@st0012 I read a bit more on thread-safety and atomics in go. From my understanding the better solution would be to change the HashObject.Pairs variable from map[string]Object to map[string]unsafe.Pointer and using the atomic Load() and Store() functions for reading and writing. Is this correct?

st0012 commented 6 years ago

@ear7h I think you can just use Go 1.9's new feature: concurrent map

64kramsystem commented 6 years ago

@st0012 If you refer to making the base Hash class access (r/w) thread-safe, I can take care of it, although, after thinking about the subject, I think it's a design decision not to underestimate. Due to, I guess, performance considerations, other languages generally have extra classes as safe counterpart, and I think it would be a good idea to do the same in Goby.

Let me know either way and I'll implement it! :smile:

64kramsystem commented 6 years ago

@st0012 How do you want to proceed with this? Create a new class, like Concurrent::Hash and Concurrent::Array?

st0012 commented 6 years ago

@saveriomiroddi Yes, I think it's the most proper way to do that.

st0012 commented 6 years ago

@goby-lang/maintainers Is there anyone want to give this a try? This should be an interesting task

64kramsystem commented 6 years ago

@st0012 depending on the timeline expected, I can have Concurrent::Hash ready by Monday, because I'm having quite busy times :smile:

I'm not 100% sure about the testing - I have fuzzy testing in mind, but the class itself should be trivial.

st0012 commented 6 years ago

@saveriomiroddi Good to hear that, no rush 👍 And I think it'll be hard to write an "must fail" test for testing concurrency. But we can run a single case multiple times to increase the odd.