RFR: 8222532: (zipfs) Performance regression when writing ZipFileSystem entries in parallel

[Claes Redestad]

Sherman,

I actually left the ExChannelCloser out as I was stepping back the
changes one by one, thinking it had nothing to do with the behavior
we're trying to restore here, but then I ran into some obscure test
failures (IIRC some zip files not being generated as expected) - when I
restored the ExChannelCloser code those tests passed.

Thus I'd like to leave it as-is for this patch to keep tests happy, but
I admit I don't fully understand what was going on in those tests. OK
to leave it for an RFE to re-examine and remove the
Ex(isting)ChannelCloser?

/Claes

On 2019-04-24 09:28, Xueming Shen wrote:
> Alan, Claes,
> 
> If my memory was correct, ExChannelClose was not being used even in 
> previous implementation.
> 
> Actually it should never have been used in any of the released JDK. I 
> might have been missing some discussions ... so I might be wrong here. 
> But the original reason of having a exchannelcloser was
> 
> that I was implementing the sync() the way that it would be invoked 
> during its lifetime. Means you
> 
> can actually fresh the in-memory entries back to underlying zipfile to 
> reduce memory pressure ...
> 
> because the fs is not really being closed in this situation, we can't 
> just close those streams. Have to
> 
> keep them alive till they get closed by the caller. But then the plan 
> was changed and the sync() is
> 
> being invoked only in its close(). In this situation, we should simply 
> close those pending streams.
> 
> The filesystem is getting closed, any inputstream open on its entry 
> should be closed as well. No
> 
> need for this closer any more in current sync() use scenario.
> 
> -Sherman
> 
> 
> 
> ExistingChannelCloser
> 
> On 4/23/19 4:42 AM, Claes Redestad wrote:
>> Hi Alan, Christoph,
>>
>> thanks for reviewing this!
>>
>> On 2019-04-23 12:42, Alan Bateman wrote:
>>> On 18/04/2019 14:10, Claes Redestad wrote:
>>>> Webrev: http://cr.openjdk.java.net/~redestad/8222532/open.00/
>>> I think the approach looks good.
>>>
>>> One thing that I think could be improved is ExChannelCloser. I think 
>>> it needs a better name. Also I think it should encapsulate its fields 
>>> a lot better so that sync doesn't need to access its interanls -- 
>>> e.g. maybe it can define a closeAndDelete method that closes the 
>>> channel and deletes the path.
>>>
>>
>> Based on what it does, ExChannelCloser (which I restored from its pre-
>> existing state) should probably be named ExistingChannelCloser.
>>
>> I've collected the feedback from you, Christoph and Lance into this
>> new webrev: http://cr.openjdk.java.net/~redestad/8222532/open.01/
>>
>> Incremental: http://cr.openjdk.java.net/~redestad/8222532/open.00_01/
>>
>> Thanks!
>>
>> /Claes