harmony-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Mikhail Markov" <mikhail.a.mar...@gmail.com>
Subject Re: [classlib][nio]FileChannel and direct buffer reallocation(was Re: [jira] Commented: (HARMONY-4081) [classlib][nio] FileChannel.write(ByteBuffer[]) sometimes works incorrectly)
Date Mon, 10 Sep 2007 14:48:57 GMT
Hi, Leo,

I've just checked only FileChannelImpl changes with the latest svn snapshot
and got VM crash (in IBM VME) in threadstart WIN API function. Could you
repeat this?

Thanks,
Mikhail

On 8/22/07, Leo Li <liyilei1979@gmail.com> wrote:
>
> Hi, Mikhail:
>     I have just focused on the problem about how to ensure direct byte
> buffer to be release in time. And after I applied the patch at r567561,
> although the problem of releasing direct byte buffer seems resolved, I
> found
> one testcase in FileChannel failed.
>     After some studying I found the problem is coincident with
> HARMONY-4081, in that there is a bug in FileChannel.write() that there is
> no
> holder for temporarily allocated direct buffers then the they might be
> gc-ed
> and the related memory resource reallocated. My patch, which  might
> intrigue GC, aggravates this problem and leads to failure in normal
> testsuite.
>     So could you please first commit the part of FileChannel.write() and
> let current tests pass?
>
> Thanks.
>
> On 8/21/07, Mikhail Markov <mikhail.a.markov@gmail.com> wrote:
> >
> > Thanks for detailed comments!
> > You are right about the memory auto-freeing, so my modifications of
> > AbstractMemorySpy are not correct.
> > See my comments for MappedByteBuffer below inlined.
> >
> > Still the changes in FileChannelImpl alone do not work: I've just
> > re-tried:
> > the test still fails and the following messages starts printing:
> > ...
> > Memory Spy! Fixed attempt to free memory that was not allocated
> > PlatformAddress[29968352]
> > ...
> > I've added debug stack-trace printing and found that these messages are
> > printed when tried to free DirectBuffers at the end of
> > FileChannelImpl.write()
> > method. It's strange at least, that we could not explicitly free
> > DirectBuffer which we allocated.
> > Seems like these buffers were freed in AbstractMemorySpy.orphanedMemory
> ()
> > method.
> > The comment for DirectByteBuffer.free() method says:
> > "Explicitly free the memory used by this direct byte buffer. If the
> memory
> > has already been freed then this is a no-op.
> > ...
> > Note this is is possible that the memory is freed by code that reaches
> > into
> > the address and explicitly frees it 'beneith' us -- this is bad form."
> > Does it mean that freeing in AbstractMemorySpy.orphanedMemory() is "bad
> > form"? :-)
> > We should somehow "synchronize" the explicit memory freeing and
> > auto-freeing
> > in AbstractMemorySpy.
> > Looking into the code, i could propose to add additional boolean
> parameter
> > to AbstractMemorySpy.free() method to indicate if warning message should
> > be
> > printed or not but the main problem here is that reproducer still fails
> if
> > modify just FileChannelImpl, which means that auto-freeing does not work
> > as
> > expected. And I'm not quite understand why it's so.
> > Do you have any ideas?
> >
> > Thanks,
> > Mikhail
> >
> >
> > On 8/17/07, Yang Paulex <paulex.yang@gmail.com> wrote:
> > >
> > > I'm forwarding this discussion to dev-list to make the discussion
> > > easier:).
> > > Please see my comments inline.
> > >
> > > 2007/8/16, Mikhail Markov (JIRA) <jira@apache.org>:
> > > >
> > > >
> > > >     [
> > > >
> > >
> >
> https://issues.apache.org/jira/browse/HARMONY-4081?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12520216
> > > ]
> > > >
> > > > Mikhail Markov commented on HARMONY-4081:
> > > > -----------------------------------------
> > > >
> > > > Paulex,
> > > >
> > > > Thanks for the patch review!
> > > >
> > > > In the beginning, i've created the patch withouth
> > MappedPlatformAddress
> > > > and AbstractMemorySpy modifications, but this lead to exceptions.
> > Seems
> > > like
> > > > after explicit temporary buffers freeing at the end of
> > > > FileChannelImpl.write() method, another attempt to free the same
> > > resources
> > > > is made in RuntimeMemorySpy.alloc() method.
> > > >
> > > > About MappedPlatformAddress modification: yes - on Linux it's as you
> > > > described, but unfortunately on Windows UnmapViewOfFile function is
> > > used,
> > > > which does not physically free the memory.
> > >
> > >
> > >
> > >
> > > I missed to mention windows implementation last time, but I don't
> catch
> > > you
> > > up here on the UnmapViewOfFile, because I cannot find the relevant
> > > explanation in MSDN that this method needs further free() to release
> > > physical memory [1], and the sample code of MSDN doesn't add any
> further
> > > memory free for this[2]. Even if UnmapViewOfFile doesn't free the
> > memory,
> > > I
> > > prefer we modify the unmap() implementation on Windows to add memory
> > free,
> > > so that the the platform neutral behavior can be kept for portlib,
> > > otherwise
> > > on Linux we may put the situation in risk that free same memory twice.
> > how
> > > do you think?
> >
> >
> > I've just checked the info again and agree with you - no explicit memory
> > freeing is needed.
> >
> > But I did see some potential problems, although not sure because MSDN is
> > not
> > > very clear here:-
> > >
> > > The CloseHandle() needs to be invoked after all file view is unmapped,
> > in
> > > our case, we don't support multi-view for same file mapping object, so
> > > it's
> > > OK to close the handle right after unmap. But for some unknown
> reasons,
> > > currently CloseHandle() is done in windows version's mmapImpl(Ln. 151,
> > > luni/src/main/native/luni/windows/OSMemoryWin32.c) right after
> > > MapViewOfFile, I'm not sure if this is right action or not. Some
> > relevant
> > > MSDN pages:
> > >
> > > "Unmapping a file view invalidates the pointer to the process's
> virtual
> > > address space. If any of the pages of the file view have changed since
> > the
> > > view was mapped, the system writes the changed pages of the file to
> disk
> > > using caching. To commit all data to disk before unmapping the file
> > view,
> > > use the *FlushViewOfFile*<
> > > http://msdn2.microsoft.com/en-us/library/aa366563.aspx>function.
> > >
> > > When each process finishes using the file mapping object and has
> > unmapped
> > > all views, it must close the file mapping object's handle and the file
> > on
> > > disk by calling
> > > *CloseHandle*<http://msdn2.microsoft.com/en-us/library/ms724211.aspx>.
> > > These calls to *CloseHandle* succeed even when there are file views
> that
> > > are
> > > still open. However, leaving file views mapped causes memory leaks."
> > > In Harmony implementation, we actually call CloseHandle before the
> only
> > > mapped file view is unmapped, but from the document above, I cannot
> say
> > > it's
> > > safe or not. I'll try to find some time to test later.
> >
> >
> > I've read in [1] in Remarks:
> > "Although an application may close the file handle used to create a file
> > mapping object, the system holds the corresponding file open until the
> > last
> > view of the file is unmapped: Files for which the last view has not yet
> > been
> > unmapped are held open with no sharing restrictions."
> >
> > So, seems like immediate closing file handles after mapping looks ok...
> >
> >
> > [1] http://msdn2.microsoft.com/en-us/library/aa366882.aspx
> > > [2] http://msdn2.microsoft.com/en-us/library/aa366548.aspx
> > > [3] http://msdn2.microsoft.com/en-us/library/aa366532.aspx
> > >
> > > About AbstractMemorySpy modification: the modification is related to
> the
> > > one
> > > > in MappedPlatformAddress. Usually, the following construction is
> used
> > > for
> > > > resources explicit freeing:
> > > >         if(memorySpy.free(this)){
> > > >             osMemory.free(osaddr);
> > > >         }
> > > > i.e. after removing the address from memoryInUse, physical freeing
> > > happens
> > > > - in this case. The only place where this was not so is
> > > > MappedPlatformAddress (at least for Windows), so after i added
> > explicit
> > > > memory freeing in MappedPlatformAddress, the address could be safely
> > > removed
> > > > from refToShadow.
> > > > You're right - is this case the mechanizm of auto-freeing is not
> > > > necessary.
> > > > I did not found places where free() method explicity used
> > > > except  *PlatformAddress classes. Do you know any?
> > > > If not then do we really need this mechanizm if all physical freeing
> > is
> > > > done in *PlatformAddress classes?
> > >
> > >
> > > PlatformAddress is used not only by MappedDirectBuffer but by common
> > > direct
> > > buffer, too. We cannot ask applications running on Harmony to
> explicitly
> > > free all direct buffer, so the automatic reallocation  mechanism is
> > still
> > > necessary.  If the number/size of direct buffer used by
> > FileChannelImpl's
> > > gather/scatter IO make you uncomfortable so that they are expected to
> be
> > > released explicitly and quickly, I prefer we find some way to deal
> with
> > > this
> > > within FileChannelImpl rather than in a method of PlatformAddress or
> > > AbstractMemorySpy, which may be depended on by other classlib and in
> > turn
> > > by
> > > applications. How do you think?
> > >
> > > --
> > > Paulex Yang
> > > China Software Development laboratory
> > > IBM
> > >
> >
>
>
>
> --
> Leo Li
> China Software Development Lab, IBM
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message