Fix memory leak #280
Fix memory leak #280
Conversation
The original hashtable is modified to accept any value type. The key is still uint64 because atomic is impossible for general key type, and it seems that we don't really need the key to be generic. A "better" hash function is added to replace the original identity hash function, which should hopefully give less collision, seems doing nothing for now because our test cases don't have much collision.
aa50e5f to
631ed8f
Compare
|
btw @elalish I am not sure why the step for hashtable is set to 127 by default. It seems that if we are using linear probing we should use 1? |
Codecov ReportBase: 91.39% // Head: 91.37% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #280 +/- ##
==========================================
- Coverage 91.39% 91.37% -0.02%
==========================================
Files 32 33 +1
Lines 3474 3490 +16
==========================================
+ Hits 3175 3189 +14
- Misses 299 301 +2
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
| inclusive_scan( | ||
| autoPolicy(meshidTable.Size()), meshidTable.GetValueStore().begin(), | ||
| meshidTable.GetValueStore().end(), meshidTable.GetValueStore().begin()); | ||
| const int numMeshIDs = meshidTable.GetValueStore().back(); |
There was a problem hiding this comment.
If you just want the number of hash table entires, can you just call Entries()?
There was a problem hiding this comment.
Oh it doesn't work, because every value in the map will be 1.
There was a problem hiding this comment.
If every value in the map is 1, why do we need a hash table? Have you tested this against the original bug repro? What is the amount of speed up? This sort of looks like it's doing even more work than the vector version, but maybe I'm missing something?
There was a problem hiding this comment.
The problem in the original issue is that, the vector being allocated has size the global mesh id counter, which can be very large even when the mesh object itself only consists of a few mesh ids. A hash table is used so we can ignore those meshids that are not present in the current mesh.
For performance, I did not measure it against the ols version. It may be a bit slower in general, but as the global counter keep increasing, it will eventually become faster than the original one and uses less memory.
There was a problem hiding this comment.
Ah, thank you, I missed what meshids was doing. Now that this is a hash table, is this scan to remove orphaned meshIDs useful anymore? I'm thinking in context of #282 and #218, that we could hit two birds with one stone by saving the transforms associated with each meshID. However, especially for #218, it would be important to keep them all rather than removing ones that don't contribute triangles.
There was a problem hiding this comment.
We can remove the scan by using the hash table index as some unique ID for each mesh id, although I am not sure how much of a difference can this make. And this will require a slightly different API. (get raw index or something)
This is probably not related to saving transforms because the hash table is just temporary, we need another thing to store them.
There was a problem hiding this comment.
Oh right, I forgot we were also using the scan to assign new ids. Thanks!
| inclusive_scan( | ||
| autoPolicy(meshidTable.Size()), meshidTable.GetValueStore().begin(), | ||
| meshidTable.GetValueStore().end(), meshidTable.GetValueStore().begin()); | ||
| const int numMeshIDs = meshidTable.GetValueStore().back(); |
There was a problem hiding this comment.
Ah, thank you, I missed what meshids was doing. Now that this is a hash table, is this scan to remove orphaned meshIDs useful anymore? I'm thinking in context of #282 and #218, that we could hit two birds with one stone by saving the transforms associated with each meshID. However, especially for #218, it would be important to keep them all rather than removing ones that don't contribute triangles.
f74f8d1 to
b3db54c
Compare
* generic hashtable implementation The original hashtable is modified to accept any value type. The key is still uint64 because atomic is impossible for general key type, and it seems that we don't really need the key to be generic. A "better" hash function is added to replace the original identity hash function, which should hopefully give less collision, seems doing nothing for now because our test cases don't have much collision. * fix elalish#245 * fixed uninitialized field * change hashtable probe step * put hash function in template parameter * fixed compiler error Co-authored-by: Emmett Lalish <elalish@gmail.com>
Fix #245. Not yet tested to see if it works on the particular example, but I think this should solve the root issue. The
meshidsis a rough estimate of the number of distinct mesh ids in the mesh, so we can initialize the hash table with large enough size and avoid many collision which will slow down the performance (especially when using CUDA because of warp divergent).