[infinispan-dev] ConcurrentMap new methods implementation

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[infinispan-dev] ConcurrentMap new methods implementation

Katia Aresti
Hi all,

Since Java 8, some new methods are available in the ConcurrentMap interface: merge, compute, computeIfAbsent, computeIfPresent, forEach, replaceAll. ConcurrentMap interface provides a default implementation.

I'm working on https://issues.jboss.org/browse/ISPN-5728 in order to provide the infinispan specific implementation. The issue here is that to make it work, these lambdas must be Serializables, so actual code using these methods and not passing serializables lambdas will break.

I see two possibilities here, please fell free to suggest any other idea.

1) Override the default implementation and specify on the release that all the lambdas must be serializables from now on ... ? 
2)  Leave the implementation of the default methods as they are and provide new methods implemented the infinispan way :
V compute(K key, V compute(K key,
                  SerializableBiFunction<? super K, ? super V, ? extends V> remappingFunction)

What do you think ?

-- Katia

_______________________________________________
infinispan-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/infinispan-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [infinispan-dev] ConcurrentMap new methods implementation

Radim Vansa
On 03/22/2017 08:46 PM, Katia Aresti wrote:

> Hi all,
>
> Since Java 8, some new methods are available in the ConcurrentMap
> interface: merge, compute, computeIfAbsent, computeIfPresent, forEach,
> replaceAll. ConcurrentMap interface provides a default implementation.
>
> I'm working on https://issues.jboss.org/browse/ISPN-5728 
> <https://issues.jboss.org/browse/ISPN-5728> in order to provide the
> infinispan specific implementation. The issue here is that to make it
> work, these lambdas must be Serializables, so actual code using these
> methods and not passing serializables lambdas will break.
>
> I see two possibilities here, please fell free to suggest any other idea.
>
> 1) Override the default implementation and specify on the release that
> all the lambdas must be serializables from now on ... ?
> 2)  Leave the implementation of the default methods as they are and
> provide new methods implemented the infinispan way :

To be consistent with Streams API, we should assume that the lambda is
serializable (and fail as gracefully as we can if it's not and should
be) and provide the SerializableX overload method to make the lambdas
serializable when the lambda is used directly on Cache. I wouldn't leave
the default implementation, because:

1) it's prone to user error, particularly during refactoring. Switching
from functional to default implementation would introduce a regression
in his app's performance
2) For local caches or local-mode operations, the lambda doesn't need to
be serializable

Radim

> V compute(K key, Vcompute(K key,
>                    SerializableBiFunction<?super K, ?super V, ?extends V> remappingFunction)
>
> What do you think ?
>
> -- Katia
>
>
> _______________________________________________
> infinispan-dev mailing list
> [hidden email]
> https://lists.jboss.org/mailman/listinfo/infinispan-dev


--
Radim Vansa <[hidden email]>
JBoss Performance Team

_______________________________________________
infinispan-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/infinispan-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [infinispan-dev] ConcurrentMap new methods implementation

Dan Berindei
Ideally I'd like to properly support non-serializable lambdas, too, by
using conditional write operations and retrying if unsuccessful. If
that turns out to be too tricky, we could also support
non-serializable lambdas for local caches only, and throw an exception
if the cache is clustered.

I'm not sure if we should overload the method (like the streams API
does) or create new methods though, because Java 9 seems to have a
problem picking the appropriate overload:

http://ci.infinispan.org/viewLog.html?buildId=52816&tab=buildResultsDiv&buildTypeId=Infinispan_MasterHotspotJdk9

Cheers
Dan


On Thu, Mar 23, 2017 at 10:42 AM, Radim Vansa <[hidden email]> wrote:

> On 03/22/2017 08:46 PM, Katia Aresti wrote:
>> Hi all,
>>
>> Since Java 8, some new methods are available in the ConcurrentMap
>> interface: merge, compute, computeIfAbsent, computeIfPresent, forEach,
>> replaceAll. ConcurrentMap interface provides a default implementation.
>>
>> I'm working on https://issues.jboss.org/browse/ISPN-5728
>> <https://issues.jboss.org/browse/ISPN-5728> in order to provide the
>> infinispan specific implementation. The issue here is that to make it
>> work, these lambdas must be Serializables, so actual code using these
>> methods and not passing serializables lambdas will break.
>>
>> I see two possibilities here, please fell free to suggest any other idea.
>>
>> 1) Override the default implementation and specify on the release that
>> all the lambdas must be serializables from now on ... ?
>> 2)  Leave the implementation of the default methods as they are and
>> provide new methods implemented the infinispan way :
>
> To be consistent with Streams API, we should assume that the lambda is
> serializable (and fail as gracefully as we can if it's not and should
> be) and provide the SerializableX overload method to make the lambdas
> serializable when the lambda is used directly on Cache. I wouldn't leave
> the default implementation, because:
>
> 1) it's prone to user error, particularly during refactoring. Switching
> from functional to default implementation would introduce a regression
> in his app's performance
> 2) For local caches or local-mode operations, the lambda doesn't need to
> be serializable
>
> Radim
>
>> V compute(K key, Vcompute(K key,
>>                    SerializableBiFunction<?super K, ?super V, ?extends V> remappingFunction)
>>
>> What do you think ?
>>
>> -- Katia
>>
>>
>> _______________________________________________
>> infinispan-dev mailing list
>> [hidden email]
>> https://lists.jboss.org/mailman/listinfo/infinispan-dev
>
>
> --
> Radim Vansa <[hidden email]>
> JBoss Performance Team
>
> _______________________________________________
> infinispan-dev mailing list
> [hidden email]
> https://lists.jboss.org/mailman/listinfo/infinispan-dev
_______________________________________________
infinispan-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/infinispan-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [infinispan-dev] ConcurrentMap new methods implementation

William Burns-3


On Thu, Mar 23, 2017 at 11:10 AM Dan Berindei <[hidden email]> wrote:
Ideally I'd like to properly support non-serializable lambdas, too, by
using conditional write operations and retrying if unsuccessful. If
that turns out to be too tricky, we could also support
non-serializable lambdas for local caches only, and throw an exception
if the cache is clustered.

The thing is the user could always do this as well. They would basically do the replace themselves and retry just like pre Java 8. To me it seems the default way these methods behave should be the most performant for the underlying implementation. I would say even in the local method we should perform the lambda under lock.

However I was thinking it might be nice to have a way to choose which, but I don't know if adding more overrides/almost same name methods is the answer. I wonder if we add a new FLAG maybe? Prefer non serializable or something?
 

I'm not sure if we should overload the method (like the streams API
does) or create new methods though, because Java 9 seems to have a
problem picking the appropriate overload:

http://ci.infinispan.org/viewLog.html?buildId=52816&tab=buildResultsDiv&buildTypeId=Infinispan_MasterHotspotJdk9

This seems like a bug in the compiler to me. Unless they have changed the spec definition they detailed in Java 8 https://docs.oracle.com/javase/specs/jls/se8/html/jls-15.html#jls-15.12.2.5
 


Cheers
Dan


On Thu, Mar 23, 2017 at 10:42 AM, Radim Vansa <[hidden email]> wrote:
> On 03/22/2017 08:46 PM, Katia Aresti wrote:
>> Hi all,
>>
>> Since Java 8, some new methods are available in the ConcurrentMap
>> interface: merge, compute, computeIfAbsent, computeIfPresent, forEach,
>> replaceAll. ConcurrentMap interface provides a default implementation.
>>
>> I'm working on https://issues.jboss.org/browse/ISPN-5728
>> <https://issues.jboss.org/browse/ISPN-5728> in order to provide the
>> infinispan specific implementation. The issue here is that to make it
>> work, these lambdas must be Serializables, so actual code using these
>> methods and not passing serializables lambdas will break.
>>
>> I see two possibilities here, please fell free to suggest any other idea.
>>
>> 1) Override the default implementation and specify on the release that
>> all the lambdas must be serializables from now on ... ?
>> 2)  Leave the implementation of the default methods as they are and
>> provide new methods implemented the infinispan way :
>
> To be consistent with Streams API, we should assume that the lambda is
> serializable (and fail as gracefully as we can if it's not and should
> be) and provide the SerializableX overload method to make the lambdas
> serializable when the lambda is used directly on Cache. I wouldn't leave
> the default implementation, because:
>
> 1) it's prone to user error, particularly during refactoring. Switching
> from functional to default implementation would introduce a regression
> in his app's performance
> 2) For local caches or local-mode operations, the lambda doesn't need to
> be serializable
>
> Radim
>
>> V compute(K key, Vcompute(K key,
>>                    SerializableBiFunction<?super K, ?super V, ?extends V> remappingFunction)
>>
>> What do you think ?
>>
>> -- Katia
>>
>>
>> _______________________________________________
>> infinispan-dev mailing list
>> [hidden email]
>> https://lists.jboss.org/mailman/listinfo/infinispan-dev
>
>
> --
> Radim Vansa <[hidden email]>
> JBoss Performance Team
>
> _______________________________________________
> infinispan-dev mailing list
> [hidden email]
> https://lists.jboss.org/mailman/listinfo/infinispan-dev
_______________________________________________
infinispan-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/infinispan-dev

_______________________________________________
infinispan-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/infinispan-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [infinispan-dev] ConcurrentMap new methods implementation

Radim Vansa
In reply to this post by Dan Berindei
On 03/23/2017 02:52 PM, Dan Berindei wrote:
> Ideally I'd like to properly support non-serializable lambdas, too, by
> using conditional write operations and retrying if unsuccessful. If
> that turns out to be too tricky, we could also support
> non-serializable lambdas for local caches only, and throw an exception
> if the cache is clustered.

At least we should voice a warning log if the operation requires to be
handled using conditional ops.

>
> I'm not sure if we should overload the method (like the streams API
> does) or create new methods though, because Java 9 seems to have a
> problem picking the appropriate overload:
>
> http://ci.infinispan.org/viewLog.html?buildId=52816&tab=buildResultsDiv&buildTypeId=Infinispan_MasterHotspotJdk9

:-/ Have you checked spec if this is a bug or if we have used
unsupported behaviour? Anyway, until we figure out how to sort this out,
we should keep the Streams API and Cache API in sync.
I think that having this as compilation issue in user code at least
forces user to define the lambda as serializable, it's not necessarily a
bad thing (though annoying).

>
> Cheers
> Dan
>
>
> On Thu, Mar 23, 2017 at 10:42 AM, Radim Vansa <[hidden email]> wrote:
>> On 03/22/2017 08:46 PM, Katia Aresti wrote:
>>> Hi all,
>>>
>>> Since Java 8, some new methods are available in the ConcurrentMap
>>> interface: merge, compute, computeIfAbsent, computeIfPresent, forEach,
>>> replaceAll. ConcurrentMap interface provides a default implementation.
>>>
>>> I'm working on https://issues.jboss.org/browse/ISPN-5728
>>> <https://issues.jboss.org/browse/ISPN-5728> in order to provide the
>>> infinispan specific implementation. The issue here is that to make it
>>> work, these lambdas must be Serializables, so actual code using these
>>> methods and not passing serializables lambdas will break.
>>>
>>> I see two possibilities here, please fell free to suggest any other idea.
>>>
>>> 1) Override the default implementation and specify on the release that
>>> all the lambdas must be serializables from now on ... ?
>>> 2)  Leave the implementation of the default methods as they are and
>>> provide new methods implemented the infinispan way :
>> To be consistent with Streams API, we should assume that the lambda is
>> serializable (and fail as gracefully as we can if it's not and should
>> be) and provide the SerializableX overload method to make the lambdas
>> serializable when the lambda is used directly on Cache. I wouldn't leave
>> the default implementation, because:
>>
>> 1) it's prone to user error, particularly during refactoring. Switching
>> from functional to default implementation would introduce a regression
>> in his app's performance
>> 2) For local caches or local-mode operations, the lambda doesn't need to
>> be serializable
>>
>> Radim
>>
>>> V compute(K key, Vcompute(K key,
>>>                     SerializableBiFunction<?super K, ?super V, ?extends V> remappingFunction)
>>>
>>> What do you think ?
>>>
>>> -- Katia
>>>
>>>
>>> _______________________________________________
>>> infinispan-dev mailing list
>>> [hidden email]
>>> https://lists.jboss.org/mailman/listinfo/infinispan-dev
>>
>> --
>> Radim Vansa <[hidden email]>
>> JBoss Performance Team
>>
>> _______________________________________________
>> infinispan-dev mailing list
>> [hidden email]
>> https://lists.jboss.org/mailman/listinfo/infinispan-dev
> _______________________________________________
> infinispan-dev mailing list
> [hidden email]
> https://lists.jboss.org/mailman/listinfo/infinispan-dev


--
Radim Vansa <[hidden email]>
JBoss Performance Team

_______________________________________________
infinispan-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/infinispan-dev
Loading...