mbeckerle commented on issue #150: Removed use of ThreadLocal for DFA Register Pool. URL: https://github.com/apache/incubator-daffodil/pull/150#issuecomment-446632842 Re: Performance. I think this change can only improve performance, because instead of calling ThreadLocal.get (which calls a hashTable get), we're just directly accessing the state we already have. I entirely agree the pools leak stuff should go. Some pools are "disciplined", and a macro that does the get and release could be created - I did a little web surfing about that. The now-called dfaRegisterPool is exactly this case. This is a replacement for the C++ use of an automatic/local variable that is a struct. That's all we're looking for with that sort of pool. We just want an object for a little local use. In this case using a macro that lays down a try/finally is actually adding a bunch of overhead we don't want. I'd prefer to have a macro that does the get and release, so you don't have to remember, but which doesn't use a try-catch, along with a checker at end of parse/unparse that does both a check to see if you've leaked them and resets the pool so there can't be subsequent interactions ever. The less-disciplined pools e.g., for marks and such in the I/O system, those are just harder, but we should actually measure if they are saving us anything. Might be better to just allocate and discard in those cases. If we create a ticket for "fix the pools" it should include measuring if there is any benefit, and just removing the pools that aren't helping. Of course something that might not be noticeable in overhead today might become so in the future.....
---------------------------------------------------------------- This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: [email protected] With regards, Apache Git Services
