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, 20 Aug 2007 16:37:32 GMT
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
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()
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?


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

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