Community
Participate
Working Groups
Currently Xtend takes code like: (from example) def create result : new NetNode() copyNet(NetNode toCopy) { result.name = toCopy.name result.references = toCopy.references.map( node | node.copyNet) } and generates code like: private final HashMap<ArrayList<?>,NetNode> _createCache_copyNet = CollectionLiterals.newHashMap(); public NetNode copyNet(final NetNode toCopy) { final ArrayList<?>_cacheKey = CollectionLiterals.newArrayList(toCopy); final NetNode _result; synchronized (_createCache_copyNet) { if (_createCache_copyNet.containsKey(_cacheKey)) { return _createCache_copyNet.get(_cacheKey); } NetNode _netNode = new NetNode(); _result = _netNode; _createCache_copyNet.put(_cacheKey, _result); } _init_copyNet(_result, toCopy); return _result; } Unfortunately I found some issues with this: - for better threading HashMap should rather be replaced with ConcurrentHashMap - 'synchronized' is left too early - other threads may get partially constructed object - if _init_copyNet throws an exception, next calls to copyNet with same arguments will give only partially constructed object - sometimes we just need to cache complex computations (eg numeric computations which return double or BigDecimal, which are immutable) - impossible with the current design My suggestion would be allowing to skip result: new Something() and changing implementation a bit: private final ConcurrentHashMap<ArrayList<?>,NetNode> _createCache_copyNet = new ConcurrentHashMap<ArrayList<?>,NetNode>; private final HashMap<ArrayList<?>,NetNode> _constructionInProgress_copyNet = new ConcurrentHashMap<ArrayList<?>,NetNode>; public NetNode copyNet(final NetNode toCopy) { final ArrayList<?>_cacheKey = CollectionLiterals.newArrayList(toCopy); final NetNode _result = _createCache_copyNet.get(_cacheKey); if(_result != null) return _result; synchronized (_createCache_copyNet) { if (_createCache_copyNet.containsKey(_cacheKey)) { return _createCache_copyNet.get(_cacheKey); } if (_constructionInProgress_copyNet.containsKey(_cacheKey)) { return _constructionInProgress_copyNet.get(_cacheKey); } NetNode _netNode = new NetNode(); _result = _netNode; _constructionInProgress_copyNet.put(_cacheKey, _result); try { _init_copyNet(_result, toCopy); } catch(RuntimeException e) { _constructionInProgress_copyNet.remove(_result) } _createCache_copyNet.put(_cacheKey, _result); } return _result; } } When the programmer doesn't specify result: new Something() (just needs caching of complex computations), we can skip _constructionInProgress_copyNet this way: private final ConcurrentHashMap<ArrayList<?>,NetNode> _createCache_copyNet = new ConcurrentHashMap<ArrayList<?>,NetNode>; public NetNode copyNet(final NetNode toCopy) { final ArrayList<?>_cacheKey = CollectionLiterals.newArrayList(toCopy); final NetNode _result = _createCache_copyNet.get(_cacheKey); if(_result != null) return _result; synchronized (_createCache_copyNet) { if (_createCache_copyNet.containsKey(_cacheKey)) { return _createCache_copyNet.get(_cacheKey); } NetNode _netNode = _init_copyNet(_result, toCopy); _createCache_copyNet.put(_cacheKey, _result); } return _result; } }
(In reply to comment #0) > Unfortunately I found some issues with this: > - for better threading HashMap should rather be replaced with ConcurrentHashMap How would that improve the situation? > - 'synchronized' is left too early - other threads may get partially > constructed object That's the idea. Recursive calls (or calls from other threads) should get a handle to the initialized object. It's not a caching mechanism but a way to write a graph transformation which would usually need two passes in one. > - if _init_copyNet throws an exception, next calls to copyNet with same > arguments will give only partially constructed object Yes, that's intended. The reference could already been used before the exception was thrown, so uninitialized objects may hang around. Better catch exceptions or invalidate the cash if one was thrown. > - sometimes we just need to cache complex computations (eg numeric computations > which return double or BigDecimal, which are immutable) - impossible with the > current design Then you wouldn't rely on recursive graph problems, which is what this was designed for. I wouldn't use this feature for that, but you could: create result : a +b addition(int a, int b) {} Tell us what you want to do, and we might be able to help you. It seems like create functions are just not the solution to your problem.
(In reply to comment #1) > > - for better threading HashMap should rather be replaced with ConcurrentHashMap > > How would that improve the situation? See lines: NetNode _result = _createCache_copyNet.get(_cacheKey); if(_result != null) return _result; Thanks to ConcurrentHashMap it's thread-safe and faster (no lock acquired if object already exists). It's just so called double-checking pattern (which since Java 1.5 finally works well) > > > - 'synchronized' is left too early - other threads may get partially > > constructed object > > That's the idea. Recursive calls (or calls from other threads) should get a > handle to the initialized object. > It's not a caching mechanism but a way to write a graph transformation which > would usually need two passes in one. Maybe it's what you intended, but it's counterintuitive: if thread A initializes the complex object and thread B has to do some part of the initialization, it works perfectly (thread B sees partially-initialized objects). But what usually will be intended by programmer is to get fully constructed objects inside thread B - the current design doesn't guarantee it, which will lead to bugs. I think that both my and your idea is a bit counterintuitive when we talk about multithreading, maybe some other redesign is needed. > > > - sometimes we just need to cache complex computations (eg numeric computations > > which return double or BigDecimal, which are immutable) - impossible with the > > current design > > Then you wouldn't rely on recursive graph problems, which is what this was > designed for. > I wouldn't use this feature for that, but you could: > > create result : a +b addition(int a, int b) {} I tried - doesn't compile. And even if it worked it just looks strange. Something like: create addition(int a, int b) { a + b } would be better, which is actually done my my last snippet. > > Tell us what you want to do, and we might be able to help you. It seems like > create functions are just not the solution to your problem. I don't want to do anything special for now. I just found those issues, and I noticed that create functions can be easily improved to add caching capabilities, which is (I think) more common than creation of complex objects. That's all.
Closing all bugs that were set to RESOLVED before Neon.0
