RE: svn commit: r1040145 - /lucene/dev/branches/branch_3x/lucene/src/java/org/apache/lucene/store/RAMDirectory.java

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

RE: svn commit: r1040145 - /lucene/dev/branches/branch_3x/lucene/src/java/org/apache/lucene/store/RAMDirectory.java

Uwe Schindler
This commit broke backwards in MockRAMDir. A fix would be to change MockRAMDir in backwards to use reflection to get the Map (like used at another place in MockRAMDir already).

Uwe

-----
Uwe Schindler
H.-H.-Meier-Allee 63, D-28213 Bremen
http://www.thetaphi.de
eMail: [hidden email]


> -----Original Message-----
> From: [hidden email] [mailto:[hidden email]]
> Sent: Monday, November 29, 2010 4:19 PM
> To: [hidden email]
> Subject: svn commit: r1040145 -
> /lucene/dev/branches/branch_3x/lucene/src/java/org/apache/lucene/store
> /RAMDirectory.java
>
> Author: shaie
> Date: Mon Nov 29 15:18:42 2010
> New Revision: 1040145
>
> URL: http://svn.apache.org/viewvc?rev=1040145&view=rev
> Log:
> LUCENE-2779: Use ConcurrentHashMap in RAMDirectory (3x)
>
> Modified:
>
> lucene/dev/branches/branch_3x/lucene/src/java/org/apache/lucene/store/
> RAMDirectory.java
>
> Modified:
> lucene/dev/branches/branch_3x/lucene/src/java/org/apache/lucene/store/
> RAMDirectory.java
> URL:
> http://svn.apache.org/viewvc/lucene/dev/branches/branch_3x/lucene/src/
> java/org/apache/lucene/store/RAMDirectory.java?rev=1040145&r1=1040144
> &r2=1040145&view=diff
> ==========================================================
> ====================
> ---
> lucene/dev/branches/branch_3x/lucene/src/java/org/apache/lucene/store/
> RAMDirectory.java (original)
> +++
> lucene/dev/branches/branch_3x/lucene/src/java/org/apache/lucene/store/
> RAMDirectory.java Mon Nov 29 15:18:42 2010
> @@ -20,8 +20,8 @@ package org.apache.lucene.store;
>  import java.io.IOException;
>  import java.io.FileNotFoundException;
>  import java.io.Serializable;
> -import java.util.HashMap;
> -import java.util.Set;
> +import java.util.Map;
> +import java.util.concurrent.ConcurrentHashMap;
>  import java.util.concurrent.atomic.AtomicLong;
>
>  import org.apache.lucene.index.IndexFileNameFilter;
> @@ -36,7 +36,7 @@ public class RAMDirectory extends Direct
>
>    private static final long serialVersionUID = 1l;
>
> -  protected HashMap<String,RAMFile> fileMap = new
> HashMap<String,RAMFile>();
> +  protected Map<String,RAMFile> fileMap = new
> ConcurrentHashMap<String,RAMFile>();
>    protected final AtomicLong sizeInBytes = new AtomicLong();
>
>    // *****
> @@ -83,25 +83,16 @@ public class RAMDirectory extends Direct
>    }
>
>    @Override
> -  public synchronized final String[] listAll() {
> +  public final String[] listAll() {
>      ensureOpen();
> -    Set<String> fileNames = fileMap.keySet();
> -    String[] result = new String[fileNames.size()];
> -    int i = 0;
> -    for(final String fileName: fileNames)
> -      result[i++] = fileName;
> -    return result;
> +    return fileMap.keySet().toArray(new String[0]);
>    }
>
>    /** Returns true iff the named file exists in this directory. */
>    @Override
>    public final boolean fileExists(String name) {
>      ensureOpen();
> -    RAMFile file;
> -    synchronized (this) {
> -      file = fileMap.get(name);
> -    }
> -    return file != null;
> +    return fileMap.containsKey(name);
>    }
>
>    /** Returns the time the named file was last modified.
> @@ -110,12 +101,10 @@ public class RAMDirectory extends Direct
>    @Override
>    public final long fileModified(String name) throws IOException {
>      ensureOpen();
> -    RAMFile file;
> -    synchronized (this) {
> -      file = fileMap.get(name);
> -    }
> -    if (file==null)
> +    RAMFile file = fileMap.get(name);
> +    if (file == null) {
>        throw new FileNotFoundException(name);
> +    }
>      return file.getLastModified();
>    }
>
> @@ -125,12 +114,10 @@ public class RAMDirectory extends Direct
>    @Override
>    public void touchFile(String name) throws IOException {
>      ensureOpen();
> -    RAMFile file;
> -    synchronized (this) {
> -      file = fileMap.get(name);
> -    }
> -    if (file==null)
> +    RAMFile file = fileMap.get(name);
> +    if (file == null) {
>        throw new FileNotFoundException(name);
> +    }
>
>      long ts2, ts1 = System.currentTimeMillis();
>      do {
> @@ -151,19 +138,18 @@ public class RAMDirectory extends Direct
>    @Override
>    public final long fileLength(String name) throws IOException {
>      ensureOpen();
> -    RAMFile file;
> -    synchronized (this) {
> -      file = fileMap.get(name);
> -    }
> -    if (file==null)
> +    RAMFile file = fileMap.get(name);
> +    if (file == null) {
>        throw new FileNotFoundException(name);
> +    }
>      return file.getLength();
>    }
>
> -  /** Return total size in bytes of all files in this
> -   * directory.  This is currently quantized to
> -   * RAMOutputStream.BUFFER_SIZE. */
> -  public synchronized final long sizeInBytes() {
> +  /**
> +   * Return total size in bytes of all files in this directory. This is
> +   * currently quantized to RAMOutputStream.BUFFER_SIZE.
> +   */
> +  public final long sizeInBytes() {
>      ensureOpen();
>      return sizeInBytes.get();
>    }
> @@ -172,14 +158,15 @@ public class RAMDirectory extends Direct
>     * @throws IOException if the file does not exist
>     */
>    @Override
> -  public synchronized void deleteFile(String name) throws IOException {
> +  public void deleteFile(String name) throws IOException {
>      ensureOpen();
>      RAMFile file = fileMap.remove(name);
> -    if (file!=null) {
> -        file.directory = null;
> -        sizeInBytes.addAndGet(-file.sizeInBytes);
> -    } else
> +    if (file != null) {
> +      file.directory = null;
> +      sizeInBytes.addAndGet(-file.sizeInBytes);
> +    } else {
>        throw new FileNotFoundException(name);
> +    }
>    }
>
>    /** Creates a new, empty file in the directory with the given name. Returns
> a stream writing this file. */
> @@ -187,14 +174,12 @@ public class RAMDirectory extends Direct
>    public IndexOutput createOutput(String name) throws IOException {
>      ensureOpen();
>      RAMFile file = newRAMFile();
> -    synchronized (this) {
> -      RAMFile existing = fileMap.get(name);
> -      if (existing!=null) {
> -        sizeInBytes.addAndGet(-existing.sizeInBytes);
> -        existing.directory = null;
> -      }
> -      fileMap.put(name, file);
> +    RAMFile existing = fileMap.remove(name);
> +    if (existing != null) {
> +      sizeInBytes.addAndGet(-existing.sizeInBytes);
> +      existing.directory = null;
>      }
> +    fileMap.put(name, file);
>      return new RAMOutputStream(file);
>    }
>
> @@ -211,12 +196,10 @@ public class RAMDirectory extends Direct
>    @Override
>    public IndexInput openInput(String name) throws IOException {
>      ensureOpen();
> -    RAMFile file;
> -    synchronized (this) {
> -      file = fileMap.get(name);
> -    }
> -    if (file == null)
> +    RAMFile file = fileMap.get(name);
> +    if (file == null) {
>        throw new FileNotFoundException(name);
> +    }
>      return new RAMInputStream(file);
>    }
>
>



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

RE: svn commit: r1040145 - /lucene/dev/branches/branch_3x/lucene/src/java/org/apache/lucene/store/RAMDirectory.java

Uwe Schindler
Attached patch fixes the backwards tests...

********* BUT ***********

The change in RAMDirectory of a *protected* field brakes backwards!!!! We are no longer binary compatible, so I don’t think the change is good in 3.x. We should do this only in trunk, so please revert the 3.x change for now or have a sophisticated backwards layer!

********* BUT ***********

Uwe

-----
Uwe Schindler
H.-H.-Meier-Allee 63, D-28213 Bremen
http://www.thetaphi.de
eMail: [hidden email]

> From: Uwe Schindler [mailto:[hidden email]]
> Sent: Monday, November 29, 2010 10:05 PM
> To: [hidden email]
> Subject: RE: svn commit: r1040145 -
> /lucene/dev/branches/branch_3x/lucene/src/java/org/apache/lucene/store
> /RAMDirectory.java
>
> This commit broke backwards in MockRAMDir. A fix would be to change
> MockRAMDir in backwards to use reflection to get the Map (like used at
> another place in MockRAMDir already).
>
> Uwe
>
> -----
> Uwe Schindler
> H.-H.-Meier-Allee 63, D-28213 Bremen
> http://www.thetaphi.de
> eMail: [hidden email]
>
>
> > -----Original Message-----
> > From: [hidden email] [mailto:[hidden email]]
> > Sent: Monday, November 29, 2010 4:19 PM
> > To: [hidden email]
> > Subject: svn commit: r1040145 -
> >
> /lucene/dev/branches/branch_3x/lucene/src/java/org/apache/lucene/store
> > /RAMDirectory.java
> >
> > Author: shaie
> > Date: Mon Nov 29 15:18:42 2010
> > New Revision: 1040145
> >
> > URL: http://svn.apache.org/viewvc?rev=1040145&view=rev
> > Log:
> > LUCENE-2779: Use ConcurrentHashMap in RAMDirectory (3x)
> >
> > Modified:
> >
> >
> lucene/dev/branches/branch_3x/lucene/src/java/org/apache/lucene/store/
> > RAMDirectory.java
> >
> > Modified:
> >
> lucene/dev/branches/branch_3x/lucene/src/java/org/apache/lucene/store/
> > RAMDirectory.java
> > URL:
> >
> http://svn.apache.org/viewvc/lucene/dev/branches/branch_3x/lucene/src/
> >
> java/org/apache/lucene/store/RAMDirectory.java?rev=1040145&r1=1040144
> > &r2=1040145&view=diff
> >
> ==========================================================
> > ====================
> > ---
> >
> lucene/dev/branches/branch_3x/lucene/src/java/org/apache/lucene/store/
> > RAMDirectory.java (original)
> > +++
> >
> lucene/dev/branches/branch_3x/lucene/src/java/org/apache/lucene/store/
> > RAMDirectory.java Mon Nov 29 15:18:42 2010 @@ -20,8 +20,8 @@ package
> > org.apache.lucene.store;  import java.io.IOException;  import
> > java.io.FileNotFoundException;  import java.io.Serializable; -import
> > java.util.HashMap; -import java.util.Set;
> > +import java.util.Map;
> > +import java.util.concurrent.ConcurrentHashMap;
> >  import java.util.concurrent.atomic.AtomicLong;
> >
> >  import org.apache.lucene.index.IndexFileNameFilter;
> > @@ -36,7 +36,7 @@ public class RAMDirectory extends Direct
> >
> >    private static final long serialVersionUID = 1l;
> >
> > -  protected HashMap<String,RAMFile> fileMap = new
> > HashMap<String,RAMFile>();
> > +  protected Map<String,RAMFile> fileMap = new
> > ConcurrentHashMap<String,RAMFile>();
> >    protected final AtomicLong sizeInBytes = new AtomicLong();
> >
> >    // *****
> > @@ -83,25 +83,16 @@ public class RAMDirectory extends Direct
> >    }
> >
> >    @Override
> > -  public synchronized final String[] listAll() {
> > +  public final String[] listAll() {
> >      ensureOpen();
> > -    Set<String> fileNames = fileMap.keySet();
> > -    String[] result = new String[fileNames.size()];
> > -    int i = 0;
> > -    for(final String fileName: fileNames)
> > -      result[i++] = fileName;
> > -    return result;
> > +    return fileMap.keySet().toArray(new String[0]);
> >    }
> >
> >    /** Returns true iff the named file exists in this directory. */
> >    @Override
> >    public final boolean fileExists(String name) {
> >      ensureOpen();
> > -    RAMFile file;
> > -    synchronized (this) {
> > -      file = fileMap.get(name);
> > -    }
> > -    return file != null;
> > +    return fileMap.containsKey(name);
> >    }
> >
> >    /** Returns the time the named file was last modified.
> > @@ -110,12 +101,10 @@ public class RAMDirectory extends Direct
> >    @Override
> >    public final long fileModified(String name) throws IOException {
> >      ensureOpen();
> > -    RAMFile file;
> > -    synchronized (this) {
> > -      file = fileMap.get(name);
> > -    }
> > -    if (file==null)
> > +    RAMFile file = fileMap.get(name);
> > +    if (file == null) {
> >        throw new FileNotFoundException(name);
> > +    }
> >      return file.getLastModified();
> >    }
> >
> > @@ -125,12 +114,10 @@ public class RAMDirectory extends Direct
> >    @Override
> >    public void touchFile(String name) throws IOException {
> >      ensureOpen();
> > -    RAMFile file;
> > -    synchronized (this) {
> > -      file = fileMap.get(name);
> > -    }
> > -    if (file==null)
> > +    RAMFile file = fileMap.get(name);
> > +    if (file == null) {
> >        throw new FileNotFoundException(name);
> > +    }
> >
> >      long ts2, ts1 = System.currentTimeMillis();
> >      do {
> > @@ -151,19 +138,18 @@ public class RAMDirectory extends Direct
> >    @Override
> >    public final long fileLength(String name) throws IOException {
> >      ensureOpen();
> > -    RAMFile file;
> > -    synchronized (this) {
> > -      file = fileMap.get(name);
> > -    }
> > -    if (file==null)
> > +    RAMFile file = fileMap.get(name);
> > +    if (file == null) {
> >        throw new FileNotFoundException(name);
> > +    }
> >      return file.getLength();
> >    }
> >
> > -  /** Return total size in bytes of all files in this
> > -   * directory.  This is currently quantized to
> > -   * RAMOutputStream.BUFFER_SIZE. */
> > -  public synchronized final long sizeInBytes() {
> > +  /**
> > +   * Return total size in bytes of all files in this directory. This is
> > +   * currently quantized to RAMOutputStream.BUFFER_SIZE.
> > +   */
> > +  public final long sizeInBytes() {
> >      ensureOpen();
> >      return sizeInBytes.get();
> >    }
> > @@ -172,14 +158,15 @@ public class RAMDirectory extends Direct
> >     * @throws IOException if the file does not exist
> >     */
> >    @Override
> > -  public synchronized void deleteFile(String name) throws IOException
> > {
> > +  public void deleteFile(String name) throws IOException {
> >      ensureOpen();
> >      RAMFile file = fileMap.remove(name);
> > -    if (file!=null) {
> > -        file.directory = null;
> > -        sizeInBytes.addAndGet(-file.sizeInBytes);
> > -    } else
> > +    if (file != null) {
> > +      file.directory = null;
> > +      sizeInBytes.addAndGet(-file.sizeInBytes);
> > +    } else {
> >        throw new FileNotFoundException(name);
> > +    }
> >    }
> >
> >    /** Creates a new, empty file in the directory with the given name.
> > Returns a stream writing this file. */ @@ -187,14 +174,12 @@ public
> > class RAMDirectory extends Direct
> >    public IndexOutput createOutput(String name) throws IOException {
> >      ensureOpen();
> >      RAMFile file = newRAMFile();
> > -    synchronized (this) {
> > -      RAMFile existing = fileMap.get(name);
> > -      if (existing!=null) {
> > -        sizeInBytes.addAndGet(-existing.sizeInBytes);
> > -        existing.directory = null;
> > -      }
> > -      fileMap.put(name, file);
> > +    RAMFile existing = fileMap.remove(name);
> > +    if (existing != null) {
> > +      sizeInBytes.addAndGet(-existing.sizeInBytes);
> > +      existing.directory = null;
> >      }
> > +    fileMap.put(name, file);
> >      return new RAMOutputStream(file);
> >    }
> >
> > @@ -211,12 +196,10 @@ public class RAMDirectory extends Direct
> >    @Override
> >    public IndexInput openInput(String name) throws IOException {
> >      ensureOpen();
> > -    RAMFile file;
> > -    synchronized (this) {
> > -      file = fileMap.get(name);
> > -    }
> > -    if (file == null)
> > +    RAMFile file = fileMap.get(name);
> > +    if (file == null) {
> >        throw new FileNotFoundException(name);
> > +    }
> >      return new RAMInputStream(file);
> >    }
> >
> >
>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [hidden email] For additional
> commands, e-mail: [hidden email]


---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

LUCENE-2779-backwardsfix.patch (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: svn commit: r1040145 - /lucene/dev/branches/branch_3x/lucene/src/java/org/apache/lucene/store/RAMDirectory.java

Shai Erera
Uwe, I'm sorry people rushed in to believe I broke backwards ... because I don't think I did. My only mistake was that I didn't run test-backwards, 'cause I didn't really think anything can break by using CHM and not HM.

fileMap was package-private until two days ago (LUCENE-2778) when I made it protected - therefore it wasn't a public API, right? If it wasn't public, we're allowed to change it right? The break was only in MockRAMDir, and even that is because I changed fileMap type from HashMap to Map, which IMO should have been defined like that from the beginning.

Perhaps we should discuss a more general problem we have w/ the backwards test (maybe in a dedicated thread) -- classes that access package-private API can break if that API changes - bw-policy-wise we're allowed to make such changes, but we cannot fix the backwards layer to compile against it properly. Your hack is really a hack, and shouldn't be there IMO. Instead, fixing backwards is as simple as re-compiling it. Something we could have done when backwards had its own 'src/java' folder which we could change for situations like that.

So the question now is - does this still count as a backwards break? I don't think so. I don't mind if we commit your hack to MockRAMDir just to avoid Hudson from failing. But we need to think how we can make backwards more resilient to changes that are allowed (such as changing package-private API).

Shai

On Tue, Nov 30, 2010 at 12:07 AM, Uwe Schindler <[hidden email]> wrote:
Attached patch fixes the backwards tests...

********* BUT ***********

The change in RAMDirectory of a *protected* field brakes backwards!!!! We are no longer binary compatible, so I don’t think the change is good in 3.x. We should do this only in trunk, so please revert the 3.x change for now or have a sophisticated backwards layer!

********* BUT ***********

Uwe

-----
Uwe Schindler
H.-H.-Meier-Allee 63, D-28213 Bremen
http://www.thetaphi.de
eMail: [hidden email]

> From: Uwe Schindler [mailto:[hidden email]]
> Sent: Monday, November 29, 2010 10:05 PM
> To: [hidden email]
> Subject: RE: svn commit: r1040145 -
> /lucene/dev/branches/branch_3x/lucene/src/java/org/apache/lucene/store
> /RAMDirectory.java
>
> This commit broke backwards in MockRAMDir. A fix would be to change
> MockRAMDir in backwards to use reflection to get the Map (like used at
> another place in MockRAMDir already).
>
> Uwe
>
> -----
> Uwe Schindler
> H.-H.-Meier-Allee 63, D-28213 Bremen
> http://www.thetaphi.de
> eMail: [hidden email]
>
>
> > -----Original Message-----
> > From: [hidden email] [mailto:[hidden email]]
> > Sent: Monday, November 29, 2010 4:19 PM
> > To: [hidden email]
> > Subject: svn commit: r1040145 -
> >
> /lucene/dev/branches/branch_3x/lucene/src/java/org/apache/lucene/store
> > /RAMDirectory.java
> >
> > Author: shaie
> > Date: Mon Nov 29 15:18:42 2010
> > New Revision: 1040145
> >
> > URL: http://svn.apache.org/viewvc?rev=1040145&view=rev
> > Log:
> > LUCENE-2779: Use ConcurrentHashMap in RAMDirectory (3x)
> >
> > Modified:
> >
> >
> lucene/dev/branches/branch_3x/lucene/src/java/org/apache/lucene/store/
> > RAMDirectory.java
> >
> > Modified:
> >
> lucene/dev/branches/branch_3x/lucene/src/java/org/apache/lucene/store/
> > RAMDirectory.java
> > URL:
> >
> http://svn.apache.org/viewvc/lucene/dev/branches/branch_3x/lucene/src/
> >
> java/org/apache/lucene/store/RAMDirectory.java?rev=1040145&r1=1040144
> > &r2=1040145&view=diff
> >
> ==========================================================
> > ====================
> > ---
> >
> lucene/dev/branches/branch_3x/lucene/src/java/org/apache/lucene/store/
> > RAMDirectory.java (original)
> > +++
> >
> lucene/dev/branches/branch_3x/lucene/src/java/org/apache/lucene/store/
> > RAMDirectory.java Mon Nov 29 15:18:42 2010 @@ -20,8 +20,8 @@ package
> > org.apache.lucene.store;  import java.io.IOException;  import
> > java.io.FileNotFoundException;  import java.io.Serializable; -import
> > java.util.HashMap; -import java.util.Set;
> > +import java.util.Map;
> > +import java.util.concurrent.ConcurrentHashMap;
> >  import java.util.concurrent.atomic.AtomicLong;
> >
> >  import org.apache.lucene.index.IndexFileNameFilter;
> > @@ -36,7 +36,7 @@ public class RAMDirectory extends Direct
> >
> >    private static final long serialVersionUID = 1l;
> >
> > -  protected HashMap<String,RAMFile> fileMap = new
> > HashMap<String,RAMFile>();
> > +  protected Map<String,RAMFile> fileMap = new
> > ConcurrentHashMap<String,RAMFile>();
> >    protected final AtomicLong sizeInBytes = new AtomicLong();
> >
> >    // *****
> > @@ -83,25 +83,16 @@ public class RAMDirectory extends Direct
> >    }
> >
> >    @Override
> > -  public synchronized final String[] listAll() {
> > +  public final String[] listAll() {
> >      ensureOpen();
> > -    Set<String> fileNames = fileMap.keySet();
> > -    String[] result = new String[fileNames.size()];
> > -    int i = 0;
> > -    for(final String fileName: fileNames)
> > -      result[i++] = fileName;
> > -    return result;
> > +    return fileMap.keySet().toArray(new String[0]);
> >    }
> >
> >    /** Returns true iff the named file exists in this directory. */
> >    @Override
> >    public final boolean fileExists(String name) {
> >      ensureOpen();
> > -    RAMFile file;
> > -    synchronized (this) {
> > -      file = fileMap.get(name);
> > -    }
> > -    return file != null;
> > +    return fileMap.containsKey(name);
> >    }
> >
> >    /** Returns the time the named file was last modified.
> > @@ -110,12 +101,10 @@ public class RAMDirectory extends Direct
> >    @Override
> >    public final long fileModified(String name) throws IOException {
> >      ensureOpen();
> > -    RAMFile file;
> > -    synchronized (this) {
> > -      file = fileMap.get(name);
> > -    }
> > -    if (file==null)
> > +    RAMFile file = fileMap.get(name);
> > +    if (file == null) {
> >        throw new FileNotFoundException(name);
> > +    }
> >      return file.getLastModified();
> >    }
> >
> > @@ -125,12 +114,10 @@ public class RAMDirectory extends Direct
> >    @Override
> >    public void touchFile(String name) throws IOException {
> >      ensureOpen();
> > -    RAMFile file;
> > -    synchronized (this) {
> > -      file = fileMap.get(name);
> > -    }
> > -    if (file==null)
> > +    RAMFile file = fileMap.get(name);
> > +    if (file == null) {
> >        throw new FileNotFoundException(name);
> > +    }
> >
> >      long ts2, ts1 = System.currentTimeMillis();
> >      do {
> > @@ -151,19 +138,18 @@ public class RAMDirectory extends Direct
> >    @Override
> >    public final long fileLength(String name) throws IOException {
> >      ensureOpen();
> > -    RAMFile file;
> > -    synchronized (this) {
> > -      file = fileMap.get(name);
> > -    }
> > -    if (file==null)
> > +    RAMFile file = fileMap.get(name);
> > +    if (file == null) {
> >        throw new FileNotFoundException(name);
> > +    }
> >      return file.getLength();
> >    }
> >
> > -  /** Return total size in bytes of all files in this
> > -   * directory.  This is currently quantized to
> > -   * RAMOutputStream.BUFFER_SIZE. */
> > -  public synchronized final long sizeInBytes() {
> > +  /**
> > +   * Return total size in bytes of all files in this directory. This is
> > +   * currently quantized to RAMOutputStream.BUFFER_SIZE.
> > +   */
> > +  public final long sizeInBytes() {
> >      ensureOpen();
> >      return sizeInBytes.get();
> >    }
> > @@ -172,14 +158,15 @@ public class RAMDirectory extends Direct
> >     * @throws IOException if the file does not exist
> >     */
> >    @Override
> > -  public synchronized void deleteFile(String name) throws IOException
> > {
> > +  public void deleteFile(String name) throws IOException {
> >      ensureOpen();
> >      RAMFile file = fileMap.remove(name);
> > -    if (file!=null) {
> > -        file.directory = null;
> > -        sizeInBytes.addAndGet(-file.sizeInBytes);
> > -    } else
> > +    if (file != null) {
> > +      file.directory = null;
> > +      sizeInBytes.addAndGet(-file.sizeInBytes);
> > +    } else {
> >        throw new FileNotFoundException(name);
> > +    }
> >    }
> >
> >    /** Creates a new, empty file in the directory with the given name.
> > Returns a stream writing this file. */ @@ -187,14 +174,12 @@ public
> > class RAMDirectory extends Direct
> >    public IndexOutput createOutput(String name) throws IOException {
> >      ensureOpen();
> >      RAMFile file = newRAMFile();
> > -    synchronized (this) {
> > -      RAMFile existing = fileMap.get(name);
> > -      if (existing!=null) {
> > -        sizeInBytes.addAndGet(-existing.sizeInBytes);
> > -        existing.directory = null;
> > -      }
> > -      fileMap.put(name, file);
> > +    RAMFile existing = fileMap.remove(name);
> > +    if (existing != null) {
> > +      sizeInBytes.addAndGet(-existing.sizeInBytes);
> > +      existing.directory = null;
> >      }
> > +    fileMap.put(name, file);
> >      return new RAMOutputStream(file);
> >    }
> >
> > @@ -211,12 +196,10 @@ public class RAMDirectory extends Direct
> >    @Override
> >    public IndexInput openInput(String name) throws IOException {
> >      ensureOpen();
> > -    RAMFile file;
> > -    synchronized (this) {
> > -      file = fileMap.get(name);
> > -    }
> > -    if (file == null)
> > +    RAMFile file = fileMap.get(name);
> > +    if (file == null) {
> >        throw new FileNotFoundException(name);
> > +    }
> >      return new RAMInputStream(file);
> >    }
> >
> >
>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [hidden email] For additional
> commands, e-mail: [hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

RE: svn commit: r1040145 - /lucene/dev/branches/branch_3x/lucene/src/java/org/apache/lucene/store/RAMDirectory.java

Uwe Schindler
--- From: Shai Erera [mailto:[hidden email]] ---
Uwe, I'm sorry people rushed in to believe I broke backwards ... because I
don't think I did. My only mistake was that I didn't run test-backwards,
'cause I didn't really think anything can break by using CHM and not HM.

fileMap was package-private until two days ago (LUCENE-2778) when I made it
protected - therefore it wasn't a public API, right? If it wasn't public,
we're allowed to change it right? The break was only in MockRAMDir, and even
that is because I changed fileMap type from HashMap to Map, which IMO should
have been defined like that from the beginning.
---

Hi Shai,

Sorry I did not know that this field was package private before, I was only
looking at the test. I am then fine with the patch. I have seen the other
mail about backwards breaks and backwards tests - here the fix for this
stupid HashMap/Map issue is easy, simply apply the BW patch attached to the
issue.

One thing: As you made the map protected in the previous issue, and it is
never changed, for API safety it should be final in all cases! Can you
change that, too. The reference to the Map should never be changed.

See also my comments to the other mail!

Uwe



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: svn commit: r1040145 - /lucene/dev/branches/branch_3x/lucene/src/java/org/apache/lucene/store/RAMDirectory.java

Shai Erera
Yes, good point - I'll make the member final. And also re-apply my patch combined w/ yours.

Thanks
Shai

On Tue, Nov 30, 2010 at 9:31 AM, Uwe Schindler <[hidden email]> wrote:
--- From: Shai Erera [mailto:[hidden email]] ---
Uwe, I'm sorry people rushed in to believe I broke backwards ... because I
don't think I did. My only mistake was that I didn't run test-backwards,
'cause I didn't really think anything can break by using CHM and not HM.

fileMap was package-private until two days ago (LUCENE-2778) when I made it
protected - therefore it wasn't a public API, right? If it wasn't public,
we're allowed to change it right? The break was only in MockRAMDir, and even
that is because I changed fileMap type from HashMap to Map, which IMO should
have been defined like that from the beginning.
---

Hi Shai,

Sorry I did not know that this field was package private before, I was only
looking at the test. I am then fine with the patch. I have seen the other
mail about backwards breaks and backwards tests - here the fix for this
stupid HashMap/Map issue is easy, simply apply the BW patch attached to the
issue.

One thing: As you made the map protected in the previous issue, and it is
never changed, for API safety it should be final in all cases! Can you
change that, too. The reference to the Map should never be changed.

See also my comments to the other mail!

Uwe



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]


Reply | Threaded
Open this post in threaded view
|

Re: svn commit: r1040145 - /lucene/dev/branches/branch_3x/lucene/src/java/org/apache/lucene/store/RAMDirectory.java

Earwin Burrfoot
In reply to this post by Shai Erera
On Tue, Nov 30, 2010 at 07:48, Shai Erera <[hidden email]> wrote:
> The break was only in MockRAMDir, and even
> that is because I changed fileMap type from HashMap to Map, which IMO should
> have been defined like that from the beginning.
As a piece of trivia.
I did some benchmarks and declaring fields with the type of a concrete
subclass, rather than interface, proved to be faster.
(On a latest 1.6 Sun JVM)
I guess that's due to JIT's failure to inline method calls on that
field (or additional class checks introduced before the short path).
Ugly :/

--
Kirill Zakharenko/Кирилл Захаренко ([hidden email])
Phone: +7 (495) 683-567-4
ICQ: 104465785

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]