Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 0.22.0
    • Fix Version/s: 0.22.0
    • Component/s: namenode
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      When the namenode is loading the image there is a significant amount of time being spent in the DFSUtil.string2Bytes. We have a very specific workload here. The path that namenode does getPathComponents for shares N - 1 component with the previous path this method was called for (assuming current path has N components).
      Hence we can improve the image load time by caching the result of previous conversion.
      We thought of using some simple LRU cache for components, but the reality is, String.getBytes gets optimized during runtime and LRU cache doesn't perform as well, however using just the latest path components and their translation to bytes in two arrays gives quite a performance boost.
      I could get another 20% off of the time to load the image on our cluster (30 seconds vs 24) and I wrote a simple benchmark that tests performance with and without caching.

      1. HDFS-1140.patch
        4 kB
        Dmytro Molkov
      2. HDFS-1140.2.patch
        13 kB
        Dmytro Molkov
      3. HDFS-1140.3.patch
        13 kB
        Dmytro Molkov
      4. HDFS-1140.4.patch
        13 kB
        Dmytro Molkov

        Activity

        Hide
        Hudson added a comment -

        Integrated in Hadoop-Hdfs-trunk-Commit #334 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Hdfs-trunk-Commit/334/)
        HDFS-1140. Speedup INode.getPathComponents. Contributed by Dmytro Molkov.

        Show
        Hudson added a comment - Integrated in Hadoop-Hdfs-trunk-Commit #334 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Hdfs-trunk-Commit/334/ ) HDFS-1140 . Speedup INode.getPathComponents. Contributed by Dmytro Molkov.
        Hide
        Konstantin Shvachko added a comment -

        I just committed this. Thank you Dmytro.

        Show
        Konstantin Shvachko added a comment - I just committed this. Thank you Dmytro.
        Hide
        Konstantin Shvachko added a comment -

        I filed HDFS-1284 and HDFS-1285 to address two other test failures. I checked javaDoc warnings locally, don't see anything related to this jira.

        Show
        Konstantin Shvachko added a comment - I filed HDFS-1284 and HDFS-1285 to address two other test failures. I checked javaDoc warnings locally, don't see anything related to this jira.
        Hide
        Todd Lipcon added a comment -

        I opened HDFS-1286. Unfortunately I'm leaving for a vacation on Tuesday and am pretty booked between now and then, so may not get to it until the end of this month. Thanks Konstantin.

        Show
        Todd Lipcon added a comment - I opened HDFS-1286 . Unfortunately I'm leaving for a vacation on Tuesday and am pretty booked between now and then, so may not get to it until the end of this month. Thanks Konstantin.
        Hide
        Konstantin Shvachko added a comment -

        Todd, thanks for looking. Here is another report that also failed the same test cases. I don't see it now on my box either, but you can check the logs to understand what is going on, and probably model. The message "The directory is already locked." means that the previous DN is still running or did not release the lock on the directory.
        If we could isolate TestFileAppend4 into a separate jira, then this one can be closed.

        Show
        Konstantin Shvachko added a comment - Todd, thanks for looking. Here is another report that also failed the same test cases. I don't see it now on my box either, but you can check the logs to understand what is going on, and probably model. The message "The directory is already locked." means that the previous DN is still running or did not release the lock on the directory. If we could isolate TestFileAppend4 into a separate jira, then this one can be closed.
        Hide
        Todd Lipcon added a comment -

        Hmm, TestFileAppend4 passes for me on a trunk checkout. It seems like some test that ran prior to it didn't close resources properly? Does it fail on your machine?

        Show
        Todd Lipcon added a comment - Hmm, TestFileAppend4 passes for me on a trunk checkout. It seems like some test that ran prior to it didn't close resources properly? Does it fail on your machine?
        Hide
        Konstantin Shvachko added a comment -

        I am not sure. My guess is based on this log.
        I am saying somebody should investigate, if possible.

        Show
        Konstantin Shvachko added a comment - I am not sure. My guess is based on this log . I am saying somebody should investigate, if possible.
        Hide
        Tsz Wo Nicholas Sze added a comment -

        > I believe this because org.apache.hadoop.hdfs.util.GSet links java.util.Map and Set in its javaDocs. ...

        Konstantin, are you sure? The GSet patch was committed in May and there is no javadoc warning for quite a few Hudson builds, e.g. this, this and this.

        Show
        Tsz Wo Nicholas Sze added a comment - > I believe this because org.apache.hadoop.hdfs.util.GSet links java.util.Map and Set in its javaDocs. ... Konstantin, are you sure? The GSet patch was committed in May and there is no javadoc warning for quite a few Hudson builds, e.g. this , this and this .
        Hide
        Konstantin Shvachko added a comment -

        Dmytro.
        There is one javaDoc warning saying

        [javadoc] javadoc: warning - Error fetching URL: http://java.sun.com/javase/6/docs/api/package-list

        I believe this because org.apache.hadoop.hdfs.util.GSet links java.util.Map and Set in its javaDocs. May be because they are not imported in the module. This is definitely not related to your patch.

        There are 4 test failures:

        • org.apache.hadoop.hdfs.TestFileAppend4.testRecoverFinalizedBlock
        • org.apache.hadoop.hdfs.TestFileAppend4.testCompleteOtherLeaseHoldersFile
        • org.apache.hadoop.hdfs.security.token.block.TestBlockToken.testBlockTokenRpc
        • org.apache.hadoop.hdfs.server.common.TestJspHelper.testGetUgi

        I see TestBlockToken and TestJspHelper failing on Hudson from time to time. We should file a jira to fix them.
        I don't see anybody reporting failure of TestFileAppend4. May be Todd should look at it, as it seems the failures are due to not closing or freeing some resources.
        Could you please investigate these issues.

        Show
        Konstantin Shvachko added a comment - Dmytro. There is one javaDoc warning saying [javadoc] javadoc: warning - Error fetching URL: http: //java.sun.com/javase/6/docs/api/ package -list I believe this because org.apache.hadoop.hdfs.util.GSet links java.util.Map and Set in its javaDocs. May be because they are not imported in the module. This is definitely not related to your patch. There are 4 test failures: org.apache.hadoop.hdfs.TestFileAppend4.testRecoverFinalizedBlock org.apache.hadoop.hdfs.TestFileAppend4.testCompleteOtherLeaseHoldersFile org.apache.hadoop.hdfs.security.token.block.TestBlockToken.testBlockTokenRpc org.apache.hadoop.hdfs.server.common.TestJspHelper.testGetUgi I see TestBlockToken and TestJspHelper failing on Hudson from time to time. We should file a jira to fix them. I don't see anybody reporting failure of TestFileAppend4. May be Todd should look at it, as it seems the failures are due to not closing or freeing some resources. Could you please investigate these issues.
        Hide
        Dmytro Molkov added a comment -

        My patch doesn't have anything to do with all of those -1s.
        I reran hudsonQA locally:

        [exec] +1 overall.
        [exec]
        [exec] +1 @author. The patch does not contain any @author tags.
        [exec]
        [exec] +1 tests included. The patch appears to include 3 new or modified tests.
        [exec]
        [exec] +1 javadoc. The javadoc tool did not generate any warning messages.
        [exec]
        [exec] +1 javac. The applied patch does not increase the total number of javac compiler warnings.
        [exec]
        [exec] +1 findbugs. The patch does not introduce any new Findbugs warnings.
        [exec]
        [exec] +1 release audit. The applied patch does not increase the total number of release audit warnings.

        Show
        Dmytro Molkov added a comment - My patch doesn't have anything to do with all of those -1s. I reran hudsonQA locally: [exec] +1 overall. [exec] [exec] +1 @author. The patch does not contain any @author tags. [exec] [exec] +1 tests included. The patch appears to include 3 new or modified tests. [exec] [exec] +1 javadoc. The javadoc tool did not generate any warning messages. [exec] [exec] +1 javac. The applied patch does not increase the total number of javac compiler warnings. [exec] [exec] +1 findbugs. The patch does not introduce any new Findbugs warnings. [exec] [exec] +1 release audit. The applied patch does not increase the total number of release audit warnings.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12448256/HDFS-1140.4.patch
        against trunk revision 959874.

        +1 @author. The patch does not contain any @author tags.

        +1 tests included. The patch appears to include 3 new or modified tests.

        -1 javadoc. The javadoc tool appears to have generated 1 warning messages.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        +1 findbugs. The patch does not introduce any new Findbugs warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        -1 core tests. The patch failed core unit tests.

        -1 contrib tests. The patch failed contrib unit tests.

        Test results: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h5.grid.sp2.yahoo.net/421/testReport/
        Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h5.grid.sp2.yahoo.net/421/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h5.grid.sp2.yahoo.net/421/artifact/trunk/build/test/checkstyle-errors.html
        Console output: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h5.grid.sp2.yahoo.net/421/console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12448256/HDFS-1140.4.patch against trunk revision 959874. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 new or modified tests. -1 javadoc. The javadoc tool appears to have generated 1 warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed core unit tests. -1 contrib tests. The patch failed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h5.grid.sp2.yahoo.net/421/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h5.grid.sp2.yahoo.net/421/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h5.grid.sp2.yahoo.net/421/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h5.grid.sp2.yahoo.net/421/console This message is automatically generated.
        Hide
        Konstantin Shvachko added a comment -

        Once Hudson approves.

        Show
        Konstantin Shvachko added a comment - Once Hudson approves.
        Hide
        Konstantin Shvachko added a comment -

        +1. Th patch looks good.
        I will commit it.

        Show
        Konstantin Shvachko added a comment - +1. Th patch looks good. I will commit it.
        Hide
        Dmytro Molkov added a comment -

        Thanks for your comments, Konstantin.
        I addressed all of them in a new version of the patch.

        Show
        Dmytro Molkov added a comment - Thanks for your comments, Konstantin. I addressed all of them in a new version of the patch.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12447511/HDFS-1140.3.patch
        against trunk revision 957669.

        +1 @author. The patch does not contain any @author tags.

        +1 tests included. The patch appears to include 3 new or modified tests.

        +1 javadoc. The javadoc tool did not generate any warning messages.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        -1 findbugs. The patch appears to introduce 3 new Findbugs warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        -1 core tests. The patch failed core unit tests.

        -1 contrib tests. The patch failed contrib unit tests.

        Test results: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h2.grid.sp2.yahoo.net/200/testReport/
        Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h2.grid.sp2.yahoo.net/200/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h2.grid.sp2.yahoo.net/200/artifact/trunk/build/test/checkstyle-errors.html
        Console output: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h2.grid.sp2.yahoo.net/200/console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12447511/HDFS-1140.3.patch against trunk revision 957669. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. -1 findbugs. The patch appears to introduce 3 new Findbugs warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed core unit tests. -1 contrib tests. The patch failed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h2.grid.sp2.yahoo.net/200/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h2.grid.sp2.yahoo.net/200/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h2.grid.sp2.yahoo.net/200/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h2.grid.sp2.yahoo.net/200/console This message is automatically generated.
        Hide
        Konstantin Shvachko added a comment -

        Some review comment:

        1. FSImage.isParent(String, String) is not used, please remove.
        2. Could you please add separators between the methods and javaDoc descriptions for the new methods if possible.
        3. INode.getPathFromComponents() should be DFSUtil.byteArray2String().
        4. TestPathComponents should use junit 4 style rather than junit 3.
        5. I'd advise to reuse U_STR instead of allocating DeprecatedUTF8 buff directly in FSImage.loadFSImage().
          In order to do that you can provide a convenience method similar to readString() or readBytes():
          static byte[][] readPathComponents(DataInputStream in) throws IOException {
            U_STR.readFields(in);
            return DFSUtil.bytes2byteArray(U_STR.getBytes(), U_STR.getLength(), (byte)Path.SEPARATOR_CHAR);
          }
          

          The idea was to remove DeprecatedUTF8 at some point, so it is better to keep this stuff in one place right after the declaration of U_STR.

        6. It does not look like FSDirectory.addToParent(String src ...) is used anywhere anymore. Could you please verify and remove it if so.
        7. Same with INodeDirectory.addToParent(String path, ...) - can we eliminate it too?
        Show
        Konstantin Shvachko added a comment - Some review comment: FSImage.isParent(String, String) is not used, please remove. Could you please add separators between the methods and javaDoc descriptions for the new methods if possible. INode.getPathFromComponents() should be DFSUtil.byteArray2String() . TestPathComponents should use junit 4 style rather than junit 3. I'd advise to reuse U_STR instead of allocating DeprecatedUTF8 buff directly in FSImage.loadFSImage() . In order to do that you can provide a convenience method similar to readString() or readBytes() : static byte [][] readPathComponents(DataInputStream in) throws IOException { U_STR.readFields(in); return DFSUtil.bytes2byteArray(U_STR.getBytes(), U_STR.getLength(), ( byte )Path.SEPARATOR_CHAR); } The idea was to remove DeprecatedUTF8 at some point, so it is better to keep this stuff in one place right after the declaration of U_STR. It does not look like FSDirectory.addToParent(String src ...) is used anywhere anymore. Could you please verify and remove it if so. Same with INodeDirectory.addToParent(String path, ...) - can we eliminate it too?
        Hide
        Dmytro Molkov added a comment -

        I removed one array copy from the routine. This should speed it up even more.
        I will submit this for tests right away.

        Show
        Dmytro Molkov added a comment - I removed one array copy from the routine. This should speed it up even more. I will submit this for tests right away.
        Hide
        Eli Collins added a comment -

        Makes sense. I was mostly curious to get your thoughts on how hard it would be to use byte[] throughout. It's probably not worth refactoring the code to have eg INode#name be an index into a path byte[].

        Show
        Eli Collins added a comment - Makes sense. I was mostly curious to get your thoughts on how hard it would be to use byte[] throughout. It's probably not worth refactoring the code to have eg INode#name be an index into a path byte[].
        Hide
        Dmytro Molkov added a comment -

        Eli, you are right, this patch moves us from more or less user friendly string passing to byte[][] passing already. However I do not really see we can avoid those copies. The first one is due to the nature of Writable, if you do not copy the stuff then the array you end up with can be the combination of the path currently read and those bytes you read before at the end of the array. You probably could expand bytes2byteArray to have offset and length inside of the byte array given to perform the split on.
        The second copy is also kind of unavoidable (or I do not know a good way to do it) since we need to end up with byte[][] array. The problem using byte[] array lies in how we traverse the tree of directories to find the INode the path points to. Eventually when you do INodeDirectory.getChildINode you need to have byte[] representation of the name of the child you are looking for.
        Right now every piece of the code inside of NameNode as far as I understand is relying on using byte[][] representation of the path where each part of it is the byte[] representation of an INode name. I am not sure how we can fix this.
        I can look into making bytes2byteArray be more flexible to get rid of one byte[] copy.

        Does all of this make sense? I will make other changes shortly.

        Show
        Dmytro Molkov added a comment - Eli, you are right, this patch moves us from more or less user friendly string passing to byte[][] passing already. However I do not really see we can avoid those copies. The first one is due to the nature of Writable, if you do not copy the stuff then the array you end up with can be the combination of the path currently read and those bytes you read before at the end of the array. You probably could expand bytes2byteArray to have offset and length inside of the byte array given to perform the split on. The second copy is also kind of unavoidable (or I do not know a good way to do it) since we need to end up with byte[][] array. The problem using byte[] array lies in how we traverse the tree of directories to find the INode the path points to. Eventually when you do INodeDirectory.getChildINode you need to have byte[] representation of the name of the child you are looking for. Right now every piece of the code inside of NameNode as far as I understand is relying on using byte[][] representation of the path where each part of it is the byte[] representation of an INode name. I am not sure how we can fix this. I can look into making bytes2byteArray be more flexible to get rid of one byte[] copy. Does all of this make sense? I will make other changes shortly.
        Hide
        Eli Collins added a comment -

        Hey Dmytro,

        Definitely an improvement. I noticed there's still a lot of copying going on, readBytes copies the strings bytes to a byte array, then bytes2byteArray copies this byte array into another byte array (it's hard for bytes2byteArray to use readBytes w/o copying). Would it make sense to go whole hog and just use the byte[] representation of a path internally? I understand that's a large change but it would remove a bunch of copies and since this change is all about using a less user-friendly abstraction in the name of reducing overhead it might be worth considering.

        • Do we need to add the new addToParent to preserve the old String-based API? Would be nice to have FSImage use a single representation of a path.
        • bytes2byteArray could use a javadoc.
        • Adding and using the following helper function as you've done with isParent would help readability.
          {{boolean isRoot(byte[][] pathComp) { return pathComp.length == 1 && pathComp[0].length == 0; }

          }}

        Show
        Eli Collins added a comment - Hey Dmytro, Definitely an improvement. I noticed there's still a lot of copying going on, readBytes copies the strings bytes to a byte array, then bytes2byteArray copies this byte array into another byte array (it's hard for bytes2byteArray to use readBytes w/o copying). Would it make sense to go whole hog and just use the byte[] representation of a path internally? I understand that's a large change but it would remove a bunch of copies and since this change is all about using a less user-friendly abstraction in the name of reducing overhead it might be worth considering. Do we need to add the new addToParent to preserve the old String-based API? Would be nice to have FSImage use a single representation of a path. bytes2byteArray could use a javadoc. Adding and using the following helper function as you've done with isParent would help readability. {{boolean isRoot(byte[][] pathComp) { return pathComp.length == 1 && pathComp[0].length == 0; } }}
        Hide
        Dmytro Molkov added a comment -

        Something changed in string to byte[][] conversion of the path since the patch was generated. Looking at it

        Show
        Dmytro Molkov added a comment - Something changed in string to byte[][] conversion of the path since the patch was generated. Looking at it
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12444464/HDFS-1140.2.patch
        against trunk revision 946488.

        +1 @author. The patch does not contain any @author tags.

        +1 tests included. The patch appears to include 3 new or modified tests.

        +1 javadoc. The javadoc tool did not generate any warning messages.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        -1 findbugs. The patch appears to introduce 3 new Findbugs warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        -1 core tests. The patch failed core unit tests.

        -1 contrib tests. The patch failed contrib unit tests.

        Test results: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h2.grid.sp2.yahoo.net/178/testReport/
        Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h2.grid.sp2.yahoo.net/178/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h2.grid.sp2.yahoo.net/178/artifact/trunk/build/test/checkstyle-errors.html
        Console output: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h2.grid.sp2.yahoo.net/178/console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12444464/HDFS-1140.2.patch against trunk revision 946488. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. -1 findbugs. The patch appears to introduce 3 new Findbugs warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed core unit tests. -1 contrib tests. The patch failed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h2.grid.sp2.yahoo.net/178/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h2.grid.sp2.yahoo.net/178/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h2.grid.sp2.yahoo.net/178/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h2.grid.sp2.yahoo.net/178/console This message is automatically generated.
        Hide
        Dmytro Molkov added a comment -

        Submitting this for tests. Can someone review the patch itself?

        Show
        Dmytro Molkov added a comment - Submitting this for tests. Can someone review the patch itself?
        Hide
        Dmytro Molkov added a comment -

        @Hairong. Well, I agree with you that conversion to String now is currently unnecessary. I guess I was trying to make an argument that potentially the format of the path in the image and the format of the path in the memory can be different, if someone changes it. In that case having a String representation in the middle might simplify things.
        Anyway, since currently the byte representation is the same it does make sense to operate on the byte arrays right from the start.
        Please see the patch attached. It doesn't convert the read bytes to string and introduces a codepath to insert a node based on the byte[][] representation array right from the start. Let me know if you have further comments.

        Show
        Dmytro Molkov added a comment - @Hairong. Well, I agree with you that conversion to String now is currently unnecessary. I guess I was trying to make an argument that potentially the format of the path in the image and the format of the path in the memory can be different, if someone changes it. In that case having a String representation in the middle might simplify things. Anyway, since currently the byte representation is the same it does make sense to operate on the byte arrays right from the start. Please see the patch attached. It doesn't convert the read bytes to string and introduces a codepath to insert a node based on the byte[][] representation array right from the start. Let me know if you have further comments.
        Hide
        Hairong Kuang added a comment -

        > two different abstractions that are communicating via String paths
        No I do not think so. What matters is the encoding. The goal is to convert what's on disk to java UTF8 encoding as stored in memory. Currently it happens that what's on disk uses the same encoding as what's in the memory. Why bother converting to String first.

        Show
        Hairong Kuang added a comment - > two different abstractions that are communicating via String paths No I do not think so. What matters is the encoding. The goal is to convert what's on disk to java UTF8 encoding as stored in memory. Currently it happens that what's on disk uses the same encoding as what's in the memory. Why bother converting to String first.
        Hide
        Dmytro Molkov added a comment -

        Well, I completely agree now, but since currently storing the image file on disc and storing in memory state are kind of two different abstractions that are communicating via String paths this seems like a clean way to get some performance improvement.

        Show
        Dmytro Molkov added a comment - Well, I completely agree now, but since currently storing the image file on disc and storing in memory state are kind of two different abstractions that are communicating via String paths this seems like a clean way to get some performance improvement.
        Hide
        Hairong Kuang added a comment -

        All paths are stored as bytes in memory. In theory, we do not need to convert bytes to string and then to bytes when loading fsimage. But this needs a lot of re-organization of our code.

        Show
        Hairong Kuang added a comment - All paths are stored as bytes in memory. In theory, we do not need to convert bytes to string and then to bytes when loading fsimage. But this needs a lot of re-organization of our code.
        Hide
        Dmytro Molkov added a comment -

        Please have a look at this patch.
        Currently I turn cache off right after image is loaded. Although it might not be totally correct. I analyzed our audit logs on the production cluster and if we would use this caching technique during runtime there would be ~30-33% hit ratio on parts of the path, which may give some improvement in performance.
        So I am open for suggestions

        I ran the benchmark tool and got these results:
        Old: 70008 vs. Cached: 41523
        which gives ~40% speedup on conversion.

        Show
        Dmytro Molkov added a comment - Please have a look at this patch. Currently I turn cache off right after image is loaded. Although it might not be totally correct. I analyzed our audit logs on the production cluster and if we would use this caching technique during runtime there would be ~30-33% hit ratio on parts of the path, which may give some improvement in performance. So I am open for suggestions I ran the benchmark tool and got these results: Old: 70008 vs. Cached: 41523 which gives ~40% speedup on conversion.

          People

          • Assignee:
            Dmytro Molkov
            Reporter:
            Dmytro Molkov
          • Votes:
            0 Vote for this issue
            Watchers:
            7 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development