i agree in general but:
On Tue, Jul 15, 2014 at 1:56 PM, Romain Manni-Bucau <[email protected]> wrote: > -0 > > personally I find this Json* API relatively wrong (starting from the > fact it inherits from Map or List so we *need* to extend an > implementation to not break upgrading jvm version , it also means we > can be immutable with java 6-7 but we'll not since you work with java > 8 since and we can't protect against its new methods). That a point in fact if have until now not fully recognized. You're right, thats very bad API design. I just tend to say that we may want to consider splitting up the implementations in a codebase per java major version :-( (Thats the point i want have good old conditional compiling :-) > > I'd 100% rely on TCKs when we'll get them to do as few as necessary to > be spec compliant on this topic (we can also check the RI on this > point). when do we get the TCKs? Have not looked into RI yet (to be uninfluenced) but think thats a good idea here. > > If you see we can improve performances doing it we should go for it, > if that's just to be nice not sure it worths it (it will create more > objects with very few gain and it is IMHO a drawback). > > About thread safety: I think we don't have to be thread safe at this level. I think we need it here because different threads can see different hashcodes otherwise (http://invalidcodeexception.com/do-immutability-really-means-thread-safety/) and thats really an issue especially if JsonValues are stored in collections. Regards Hendrik > > does it make sense in your opinion? > > > > Romain Manni-Bucau > Twitter: @rmannibucau > Blog: http://rmannibucau.wordpress.com/ > LinkedIn: http://fr.linkedin.com/in/rmannibucau > Github: https://github.com/rmannibucau > > > 2014-07-15 13:39 GMT+02:00 Hendrik Dev <[email protected]>: >> To open another discussion ;-) i looked on the JsonValue >> implementations regarding immutability (as the API requires). >> >> Looks like that doing real immutability have a really bad performance >> impact (at least in my prototype implementation which is here >> https://github.com/salyh/fleece_master/commit/8ce227915ef5bcaa695fa354b7126762a2221d35). >> So i understand why the initial implementation deals with the >> JsonArrayImpl.addInternal() and JsonObjectImpl.putInternal(). But if i >> were a nitpicker i would say thats not immutable :-) >> The main performance impact is due to the builder implementations i >> think if the values are really immutable. >> >> But at least the JsonValue implementations itself can be immutable >> without big perf drawbacks hopefully. To make them "more" immutable >> (and keep the put/addInternal()) i suggest: >> >> For JsonArrayImpl >> - getValuesAs() should return unmodifiable list >> https://github.com/salyh/fleece_master/commit/8ce227915ef5bcaa695fa354b7126762a2221d35#commitcomment-7011110 >> >> - subList(int fromIndex, int toIndex) should return unmodifiable list >> https://github.com/salyh/fleece_master/commit/8ce227915ef5bcaa695fa354b7126762a2221d35#commitcomment-7011145 >> >> - listIterator(int index) disallow to change list >> https://github.com/salyh/fleece_master/commit/8ce227915ef5bcaa695fa354b7126762a2221d35#commitcomment-7011156 >> >> - make class final >> >> - make hashcode caching threadsafe (int terms of "volatile") >> https://github.com/salyh/fleece_master/commit/8ce227915ef5bcaa695fa354b7126762a2221d35#commitcomment-7011019 >> >> - make addInternal package private >> >> >> For JasonObjectImpl >> - keySet() should return unmodifiable set >> >> - values() should return unmodifiable collection >> >> - entrySet() should return unmodifiable set (tricky -> >> http://grepcode.com/file/repository.grepcode.com/java/root/jdk/openjdk/7-b147/java/util/Collections.java#1366) >> >> - make class final >> >> - make hashcode caching threadsafe (int terms of "volatile") >> https://github.com/salyh/fleece_master/commit/8ce227915ef5bcaa695fa354b7126762a2221d35#commitcomment-7011019 >> >> - make putInternal package private >> >> >> For the other JsonValue implementations >> - make classes final >> - make hashcode caching threadsafe (int terms of "volatile") >> https://github.com/salyh/fleece_master/commit/8ce227915ef5bcaa695fa354b7126762a2221d35#commitcomment-7011019 >> >> Your opinion? >> >> Regards >> Hendrik >> >> >> >> -- >> Hendrik Saly (salyh, hendrikdev22) >> @hendrikdev22 >> PGP: 0x22D7F6EC -- Hendrik Saly (salyh, hendrikdev22) @hendrikdev22 PGP: 0x22D7F6EC
