Attempt to fix PR7157 (too many minor collections)#488
Conversation
|
I've updated this to use Alain's patch from the Mantis issue. I forgot there was an explicit element_size already. |
|
Except that Alain's patch doesn't fix all the occurrences. Sigh. One moment |
|
Patch updated again. |
I only tested my (partial) patch on the specific benchmark and recovered the original performances. I think it's better to use the element_size, so as to keep the generality of the new implementation (allowing to define tables with elements of different sizes). |
|
I'm looking at the testsuite failure |
|
@bobot I can't see what's wrong with this. My suspicion is that the change to make the ephemeron ref_table function correctly has exposed some other, possibly unrelated, bug. Can you have a look? |
|
OK I will. Today I hope. |
|
I think there is still a problem with the generic code for ref_table: bobot/ocaml@b471067 adds assertions about an invariant of the |
|
In fact after more debugging, @mshinwell is right it is another bug: weak.ml is using |
|
The commit bobot/ocaml@c1801a8 should fix the other bug. @mshinwell you can take it with its parent (bobot/ocaml@b471067) or not (at the end it adds only one assert, the other is removed by the next commit). The testsuite is now ok. |
60edfc5 to
4c6acde
Compare
4c6acde to
fb84058
Compare
|
I've merged the changes from @bobot, and will now merge the whole thing. |
GPR#488: [Attempt to] Fix PR7157 (too many minor collections)
d94f1da appears to have introduced a regression, with increased numbers of minor collections.
In the function realloc_generic_table there is pointer arithmetic, at first sight unchanged by this patch, that starts from e.g. "tbl->base". The units of the arithmetic will be the size of the underlying type (T when tbl->base has type T* ). In the 4.03 branch, T is just "char" (in the patch it was "void", but the common interpretation of that having the same size as "char" would have also produced the bug). As such, the result of calculations that add to the base pointer would be rather smaller than prior to the above patch, when the underlying type was "value*". Consequentially the ref table would become full more often and trigger more minor GCs (from line 510 of byterun/minor_gc.c).
This patch changes the underlying type to "void*" instead, which should restore the old behaviour.
I haven't yet tested this since others should probably give opinions first.