search

GemTalk / PharoGemStoneFFI

GemStone GCI access via Pharo FFI
MIT License
1 stars 4 forks source link

[#13] Enable caching of callout methods #14

Open akgrant43 opened 1 year ago

akgrant43 commented 1 year ago

Hi Norm, The PR is a bit messy at the moment as the modified classes use the Pharo tonal encoding, i.e. class and category names are Symbols instead of Strings.

Would you mind checking the functionality, and if you're happy to accept it I'll convert the encoding.

Thanks, Alistair

nrgiii commented 1 year ago

@akgrant43, thanks for submitting the PR. I've not been down in this code for a long time and have therefore asked @martinmcclure to review it.

martinmcclure commented 1 year ago

Hi Alistair, Thanks for the PR.

I noticed the line ending confusion in your commits -- that was our fault; the repo should have had normalized line endings but did not (#15). I've fixed that, so you can base your changes on a properly normalized repo.

Re the Tonel encoding, if you load RSR before writing out (or at least the TonelFixes package from the RSR repo), they will be written in the Tonel format we're using.

Re your changes, the real problem appears to be that uFFI is not designed to handle more than one library concurrently loaded by the same class, which has been a goal of this project.

Reading your code, it looks likely to work. Do you have any tests where multiple GemStone versions are being concurrently used?

Your way of handling concurrent multiple libraries should perform better than the original way when only one version is in use, but when multiple GciInterface instances are in use, with different libraries, it could end up thrashing the method dictionary. Probably an improvement over the current way, though.

akgrant43 commented 1 year ago

Hi Martin, I've changed the line ending back to unix format.

I don't have easy access to a 3.6 system at the moment, and I'm about to go on leave for a few weeks. Will you be at ESUG this year? If so, perhaps we could get together and test this then?

I'm not sure how this implementation will thrash the method dictionary any more than the original implementation. The original implementation forced the method to be recompiled on every call, even if the GS version was unchanged. This only recompiles when the GS version changes, so in the worst case, switching versions on alternating calls, it will be the same as the original implementation. My understanding is that VW doesn't support multiple version simultaneously, so it seems unlikely that switching versions that often is the norm.