johnzon-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Hendrik Dev <hendrikde...@gmail.com>
Subject Re: JsonValue immutability
Date Tue, 15 Jul 2014 12:37:32 GMT
i agree in general but:


On Tue, Jul 15, 2014 at 1:56 PM, Romain Manni-Bucau
<rmannibucau@gmail.com> 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 <hendrikdev22@gmail.com>:
>> 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

Mime
View raw message