In the CRC32 layer, the "optimization" was really just laziness. The use case we have only needs constant known explicit lengths, so we wanted to hard wire that, and not bother to support nor test any other cases.
But, had that worked, I would bet we would have made the mistake and pre-allocated and saved the buffer of that size, resulting in non-thread-safe code. But turns out there's a base class that handles all this for us, and we get the ability for the explicit length to be computed from an expression also, for free. I'm thinking the layer framework needs to allocate the layer transformer instance in a thread-local manner so that if it constructs mutable state that will be thread local also, because the natural thing to do is to look at the layerLength information and allocate a buffer in a member of the class. It's just the first and most obvious thing to do, and it's dead wrong for thread safety. On Wed, Aug 16, 2023 at 9:48 AM Steve Lawrence <slawre...@apache.org> wrote: > I don't remember why NoCacheEvluatable was used, but you're I suspect > you're right that it was done so implementations are less likely to mess > things up. Note that NoCacheEvaluatable is also used for > ImplicitLengthEv. I'm not sure why it's used there either. Maybe > LayerLengthEv just copied from that? > > Note that using NoCacheEvaluatble still doesn't really ensure > thread-safe implementations. An implementation could just as easily > allocate a buffer based on the first layer length it gets and then > stores that buffer for reuse later (with some logic to grow it if a > future layer length is bigger). That wouldn't be thread safe regardless > of no-cache or not. > > So I don't think we should be relying on NoCacheEvalatable to ensure > thread safe implementations. API changes seems reasonable if we want to > enforce that, or maybe we just to improve the documentation about what > parts of the layer API are thread-safe and when things can and cannot be > shared. > > That said, I'm curious what optimizations the CRC32 layer is trying to > do with a constant layer length? > > On 2023-08-15 05:49 PM, Mike Beckerle wrote: > > Lola Kilo and I were debugging a layer for CRC32 and found that the > > LayerLengthEv is a NoCacheEvaluatable, so even if the schema does > > dfdlx:layerLength="1240" a constant, the the layerLengthEv.optConstant is > > None, which is because it is a NoCacheEvaluatable, which insists > > optConstant is false to prevent constant optimization. This results in > > calling on the LayerLengthEv at runtime every time. > > > > I am not sure I understand the entire rationale for LayerLengthEv to be a > > NoCacheEvaluatable. > > > > I can think of one reason why.... so that people can't allocate mutable > > things based on that length, and store them in the layer transformer > class > > instance. Because that is a shared runtime structure, so that wouldn't be > > thread safe. If they are forced to evaluate the layerLengthEv every time, > > then their code will naturally allocate any data structure based on the > > length at runtime rather than at layer compile time. > > > > In other words, it's a hack to trick people into being more likely to > write > > thread-safe code. > > > > Based on this (and other experience with it) I think the layer API is in > > serious need of revisiting. > > > > We really should NOT just cast existing practice into JAPI/SAPI, but > should > > rethink so that, for example, a thread-local is created from the layer > > transformer instance. > > > > Comments? > > > >
