On 21 Nov 2011, at 18:01, Mircea Markus wrote: > Hi, > > The DistributionInterceptor can be simplified significantly now that we are supporting transactional or non transactional caches. > Looking at it I've found an significant performance issues though: seems like for each put we do, if the unreliableReturnValues is disabled (that's the default, i.e. disabled) then we do a remote get and then a put, i.e. 2 RPCs to the same remote node. That seems to be highly inefficient so just wondering is there any reason not to piggyback the return value on the put itself? Agreed, but this will involve separate PutKeyValueCommands for replicated vs. distributed mode. Unless you pass in a flag to the command.

On 21 Nov 2011, at 22:33, Mircea Markus wrote: > > On 21 Nov 2011, at 22:26, Manik Surtani wrote: > >> >> On 21 Nov 2011, at 18:01, Mircea Markus wrote: >> >>> Hi, >>> >>> The DistributionInterceptor can be simplified significantly now that we are supporting transactional or non transactional caches. >>> Looking at it I've found an significant performance issues though: seems like for each put we do, if the unreliableReturnValues is disabled (that's the default, i.e. disabled) then we do a remote get and then a put, i.e. 2 RPCs to the same remote node. That seems to be highly inefficient so just wondering is there any reason not to piggyback the return value on the put itself? >> >> Agreed, but this will involve separate PutKeyValueCommands for replicated vs. distributed mode. Unless you pass in a flag to the command. > it's the return value what matters, we shouldn't need different commands for that. The marshalling code decides which return values to marshall and which to ignore since it knows it won't be used. This is based on command type.

> But that's a impl detail and this is a seriously important optimisation, as it potentially adds 1 RPC for each write in the default config. Agreed, so lets see a patch - if it isn't too intrusive in terms of changing too much stuff, I'm fine with it.

On 21 Nov 2011, at 22:50, Mircea Markus wrote: > looks like the marshaller should be amended then. But not for REPL mode (or DIST with SKIP_REMOTE_LOOKUPS). Since that is unnecessary (and expensive) marshalling of return values.