harmony-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Yang Paulex" <paulex.y...@gmail.com>
Subject [classlib][nio]FileChannel and direct buffer reallocation(was Re: [jira] Commented: (HARMONY-4081) [classlib][nio] FileChannel.write(ByteBuffer[]) sometimes works incorrectly)
Date Fri, 17 Aug 2007 03:18:27 GMT
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?

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.

[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?

> [classlib][nio] FileChannel.write(ByteBuffer[]) sometimes works
> incorrectly
> >
> ---------------------------------------------------------------------------
> >
> >                 Key: HARMONY-4081
> >                 URL: https://issues.apache.org/jira/browse/HARMONY-4081
> >             Project: Harmony
> >          Issue Type: Bug
> >          Components: Classlib
> >         Environment: Windows
> >            Reporter: Vera Petrashkova
> >            Assignee: Alexei Zakharov
> >            Priority: Minor
> >         Attachments: H-4081_fix.patch, H-4081_temp_workaround.patch,
> SourceViewScreenshot-1.jpg
> >
> >
> > According to J2SE specification
> > "File channels are safe for use by multiple concurrent threads."
> > But the following test demonstrates that FileChannel.write(ByteBuffer[])
> sometimes works incorrectly when
> > some threads try to write bytes to the same channel.
> > Not all data are written to the file.
> > ---------------fchTest.java---------------
> > import java.io.File;
> > import java.io.IOException;
> > import java.io.FileOutputStream;
> > import java.io.FileInputStream;
> > import java.nio.channels.FileChannel;
> > import java.nio.ByteBuffer;
> > public class fchTest {
> >
> >     public static int N_TH = 20;
> >     public static final int BUF_SIZE = 2000;
> >     public static final int N_BUF = 20;
> >     public static final int N_WRITES = 20;
> >     boolean passed = true;
> >
> >     String fileName = "FileChannel.file";
> >     FileChannel outChannel = null;
> >     FileChannel inChannel = null;
> >     public static void main(String[] args) {
> >         try {
> >              if (args.length > 0) {
> >                 N_TH = Integer.parseInt(args[0]);
> >             }
> >         } catch (Throwable e) {
> >         }
> >         int res = new fchTest().test(args);
> >         System.err.println(res == 104 ? "Test passed" : "Test failed");
> >     }
> >     public int test(String[] params) {
> >         File f = null;
> >         passed = true;
> >         try {
> >             f = new File(fileName);
> >             if (f.exists()) {
> >                 f.delete();
> >             }
> >             outChannel = new FileOutputStream(f).getChannel();
> >             inChannel = new FileInputStream(f).getChannel();
> >             Thread[] t = new Thread[N_TH];
> >             for (int i = 0; i < t.length; ++i) {
> >                 t[i] = new thWriter(this);
> >                 t[i].start();
> >             }
> >             for (int i = 0; i < t.length; ++i) {
> >                 t[i].join();
> >             }
> >         } catch (Throwable t) {
> >             t.printStackTrace();
> >             return 105;
> >         } finally {
> >             try {
> >                 if (outChannel != null){
> >                     outChannel.close();
> >                 }
> >                 if (inChannel != null){
> >                     inChannel.close();
> >                 }
> >             } catch (Throwable t){
> >                 t.printStackTrace();
> >             }
> >             if (f != null){
> >                 f.delete();
> >             }
> >         }
> >         return (passed ? 104 : 106);
> >     }
> > }
> > class thWriter extends Thread {
> >     FileChannel outChannel = null;
> >     fchTest base = null;
> >
> >     thWriter(fchTest base) {
> >         this.outChannel = base.outChannel;
> >         this.base = base;
> >     }
> > public void run () {
> >         try {
> >             long allW = 0;
> >             long allC = 0;
> >             for (int i = 0; i < fchTest.N_WRITES; ++i) {
> >                 ByteBuffer[] bbb = createByteChunks();
> >                 for (int t = 0; t < bbb.length; t++)  {
> >                     allC += bbb[i].capacity();
> >                 }
> >                 long written = outChannel.write(bbb);
> >                 if (written != (long)((fchTest.BUF_SIZE)*fchTest.N_BUF))
> {
> >                     System.err.println(this+"  Written:
> "+written+"  should be "+ fchTest.BUF_SIZE*fchTest.N_BUF+" outChannel
> position: "+outChannel.position());
> >                     base.passed = false;
> >                 }
> >                 allW+=written;
> >                 Thread.yield();
> >                 Thread.sleep(10);
> >             }
> >             System.err.println(this+" - after write: "+allW+"  "+allC);
> >             if (allW != allC) {
> >                 base.passed = false;
> >             }
> >             outChannel.force(false);
> >         } catch (Throwable e){
> >             System.err.println(this+" unexpected exception " + e);
> >             e.printStackTrace();
> >             base.passed = false;
> >         }
> >     }
> >     ByteBuffer[] createByteChunks() {
> >         ByteBuffer[] bb_arr = new ByteBuffer[fchTest.N_BUF];
> >         byte [] bb = new byte[fchTest.BUF_SIZE];
> >         for (int i = 0; i < bb.length; i++) {
> >             bb[i] = (byte)i;
> >         }
> >         for (int i = 0; i < bb_arr.length; ++i) {
> >             bb_arr[i] = ByteBuffer.allocateDirect(bb.length).wrap(bb);
> >         }
> >         return bb_arr;
> >     }
> > }
> > ----------------------------------------------------
> > Run this test several times and change the number of created threads
> > java fchTest
> > java fchTest 30
> > Output on RI:
> > ==============
> > java version "1.5.0_06"
> > Java(TM) 2 Runtime Environment, Standard Edition (build 1.5.0_06-b05)
> > Java HotSpot(TM) Client VM (build 1.5.0_06-b05, mixed mode)
> > Thread[Thread-1,5,main] - after write: 800000  800000
> > Thread[Thread-11,5,main] - after write: 800000  800000
> > Thread[Thread-19,5,main] - after write: 800000  800000
> > Thread[Thread-17,5,main] - after write: 800000  800000
> > Thread[Thread-9,5,main] - after write: 800000  800000
> > Thread[Thread-13,5,main] - after write: 800000  800000
> > Thread[Thread-15,5,main] - after write: 800000  800000
> > Thread[Thread-7,5,main] - after write: 800000  800000
> > Thread[Thread-3,5,main] - after write: 800000  800000
> > Thread[Thread-0,5,main] - after write: 800000  800000
> > Thread[Thread-5,5,main] - after write: 800000  800000
> > Thread[Thread-16,5,main] - after write: 800000  800000
> > Thread[Thread-6,5,main] - after write: 800000  800000
> > Thread[Thread-12,5,main] - after write: 800000  800000
> > Thread[Thread-2,5,main] - after write: 800000  800000
> > Thread[Thread-8,5,main] - after write: 800000  800000
> > Thread[Thread-10,5,main] - after write: 800000  800000
> > Thread[Thread-4,5,main] - after write: 800000  800000
> > Thread[Thread-14,5,main] - after write: 800000  800000
> > Thread[Thread-18,5,main] - after write: 800000  800000
> > Test passed
> > Output on DRLVM:
> > ==============
> > Apache Harmony Launcher : (c) Copyright 1991, 2006 The Apache Software
> Foundation or its licensors, as applicable.
> > java version "1.5.0"
> > pre-alpha : not complete or compatible
> > svn = r544707, (Jun  6 2007), Windows/ia32/msvc 1310, release build
> > http://harmony.apache.org
> > The GC did not provide gc_add_weak_root_set_entry()
> > Thread[Thread-9,5,main]  Written: 33100  should be 40000 outChannel
> position: 6874000
> > Thread[Thread-20,5,main] - after write: 800000  800000
> > Thread[Thread-16,5,main] - after write: 800000  800000
> > Thread[Thread-8,5,main] - after write: 800000  800000
> > Thread[Thread-12,5,main] - after write: 800000  800000
> > Thread[Thread-22,5,main] - after write: 800000  800000
> > Thread[Thread-14,5,main] - after write: 800000  800000
> > Thread[Thread-18,5,main] - after write: 800000  800000
> > Thread[Thread-23,5,main] - after write: 800000  800000
> > Thread[Thread-11,5,main] - after write: 800000  800000
> > Thread[Thread-10,5,main] - after write: 800000  800000
> > Thread[Thread-24,5,main] - after write: 800000  800000
> > Thread[Thread-7,5,main] - after write: 800000  800000
> > Thread[Thread-15,5,main] - after write: 800000  800000
> > Thread[Thread-9,5,main] - after write: 793100  800000
> > Thread[Thread-6,5,main] - after write: 800000  800000
> > Thread[Thread-25,5,main] - after write: 800000  800000
> > Thread[Thread-17,5,main] - after write: 800000  800000
> > Thread[Thread-13,5,main]  Written: 37700  should be 40000 outChannel
> position: 15872000
> > Thread[Thread-19,5,main] - after write: 800000  800000
> > Thread[Thread-13,5,main] - after write: 797700  800000
> > Thread[Thread-21,5,main] - after write: 800000  800000
> > Test failed
> > This bug causes the failure of the test
> >     api.nio.channels.filechannel.FileChannelThrSafetyTest
> > from Reliability test suite
> https://issues.apache.org/jira/browse/HARMONY-2918
>
> --
> This message is automatically generated by JIRA.
> -
> You can reply to this email to add a comment to the issue online.
>
>


-- 
Paulex Yang
China Software Development laboratory
IBM

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