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

Fix issue https://github.com/yuin/gopher-lua/issues/214 #515

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

VigorFox
Copy link

replace golang native map with swiss map for table hash parts

Fixes #214

@tul
Copy link
Contributor

tul commented Jan 10, 2025

Is there any bench mark data available for showing performance of new implementation vs old?

@VigorFox
Copy link
Author

Is there any bench mark data available for showing performance of new implementation vs old?

The swiss map bench is in the link blow, in Figure 5.
https://medium.com/plain-golang-tutorial/swisstable-a-high-performance-hash-table-implementation-3e13bfe8c79b

My teammate also did a bench between c base lua 5.1, original gopher-lua and this fix. Here's the result:

lua 5.1:

1 million:
AverageInsertTime: 0.375600 S
AverageRemoveTime: 3.060500 S

10 million:
AverageInsertTime: 6.245700 S
AverageRemoveTime: 1.098400 S

20 million:
AverageInsertTime: 13.224200 S
AverageRemoveTime: 1.907600 S

40 million:
AverageInsertTime: 30.702400 S
AverageRemoveTime: 10.838000 S

60 million:
AverageInsertTime: 39.541000 S
AverageRemoveTime: 16.512800 S


original gopher-lua:

1 million:
AverageInsertTime: 1.607991 S
AverageRemoveTime: 0.394781 S

10 million:
AverageInsertTime: 17.648665 S
AverageRemoveTime: 4.566295 S

20 million:
AverageInsertTime: 36.088988 S
AverageRemoveTime: 9.103840 S

40 million:
AverageInsertTime: 110.276513 S
AverageRemoveTime: 26.073281 S

  • can't bench the 60 million insertion due to the leak is caused too much memory

gopher-lua with swiss map:

1 million:
AverageInsertTime: 0.632160 S
AverageRemoveTime: 0.428485 S

10 million:
AverageInsertTime: 8.247353 S
AverageRemoveTime: 4.715436 S

20 million:
AverageInsertTime: 19.227456 S
AverageRemoveTime: 10.600976 S

40 million:
AverageInsertTime: 40.685055 S
AverageRemoveTime: 23.205350 S

60 million:
AverageInsertTime: 77.476260 S
AverageRemoveTime: 34.810059 S

@tul
Copy link
Contributor

tul commented Jan 13, 2025

Thank you for taking the time to provide those figures.

Overall the figures look really good to me. Although I guess they don’t bench the iteration performance; which may be worth a test as the Swiss map impl has to do more work than the original gopher lua one in this area, given gopher lua was able to use the k2i map. However, the k2i map was also the root of the memory leak, so it could be argued it’s not a fair comparison anyway.

Overall, this looks really interesting, I’ll certainly consider using this in future, regardless of whether it’s merged to mainline here.

Thanks for sharing.

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.

table map memery leak
2 participants