Drop usage of FileInput/OutputStreams - modules a* -> c* #295

mdindoffer · 2017-11-26T20:37:23Z

This change drops the usage of FileInputStream and FileOutputStream
in cluster modules "a*" through "c*".
The FIS/FOS are putting unnecesary pressure on the GC because of their finalize() method.
I've measured small, but fairly consistent improvements in GC pauses during the startup of the IDE with couple of projects opened:
Before: http://gceasy.io/my-gc-report.jsp?p=c2hhcmVkLzIwMTcvMTEvMjYvLS1nYXJiYWdlLWNvbGxlY3Rpb24tYmVmb3JlLXN0YXJ0dXAubG9nLS0yMC0xMS01OQ==
After: http://gceasy.io/my-gc-report.jsp?p=c2hhcmVkLzIwMTcvMTEvMjYvLS1nYXJiYWdlLWNvbGxlY3Rpb24tYWZ0ZXIxY29tbWl0LXN0YXJ0dXAubG9nLS0yMC0xMy01Mw==
In every module that required it (for the "Files" class workaround), theres a bump in javac source to version 7. Where appropriate I took the liberty of using try with resources blocks.
I may have also fixed a resource leak somewhere in there.
Further references: see JDK-8080225, HDFS-8526, JENKINS-42934

emilianbold

It looks pretty. Was it manually done or do you have some .hints file to apply?

I wouldn't apply these alphabetically but per cluster or somewhat more logically.

For example, the theoretical speedup might only have happened due to a change in the core Platform modules.

Personally I would also split in separate patches the try-with-resources (which I like) and the java.nio.file.File usage (which I understand it must be more efficient but don't like so much).

emilianbold · 2017-11-26T21:50:05Z

...ort.ant/src/org/netbeans/modules/apisupport/project/universe/PlatformLayersCacheManager.java

            }
+            // corrupted cache file, throw the cache away


This comment seems to belong in the catch ClassNotFoundException block.

emilianbold · 2017-11-26T22:11:49Z

autoupdate.services/libsrc/org/netbeans/updater/UpdaterInternal.java

-                        XMLUtil.LOG.log(Level.FINE, "Closing output stream for {0}", file);
-                        super.close();
-                        XMLUtil.LOG.log(
-                            Level.INFO, "File installed {0}@{1}", 


This logger here seems useful.

emilianbold · 2017-11-26T22:13:28Z

...date.services/src/org/netbeans/modules/autoupdate/updateprovider/AutoupdateCatalogCache.java

@@ -155,24 +156,24 @@ public void storeLicense(String name, String content) {

    private String readLicenseFile(String name) {
        File file = getLicenseFile(name);
-        FileInputStream fr = null;
+        InputStream is = null;


Why not preserve the fr name?

Because the "fr" implies FileReader, which it is not. The fr naming is not really consistent with the naming in the rest of the modules I made changes at.

emilianbold · 2017-11-26T22:14:10Z

...date.services/src/org/netbeans/modules/autoupdate/updateprovider/AutoupdateCatalogCache.java

                    try {
-                        fr.close();
+                        is.close();


No try-with-resources here?

emilianbold · 2017-11-26T22:22:27Z

core.startup/src/org/netbeans/core/startup/logging/NbLogging.java

@@ -43,8 +45,9 @@
            try {
                File debugLog = new File(System.getProperty("java.io.tmpdir"), "TopLogging.log"); // NOI18N
                System.err.println("Logging sent to: " + debugLog); // NOI18N
+                Files.newOutputStream(debugLog.toPath(), StandardOpenOption.APPEND);


This line seems wrong.

lbruun · 2017-11-27T06:45:11Z

I notice your tests use:

-XX:+UseCompressedClassPointers 
-XX:+UseCompressedOops 
-XX:-UseLargePagesIndividualAllocation 
-XX:+UseParallelGC

Why? Does your test results depend on this?

lbruun · 2017-11-27T07:12:11Z

One more thing: If Files.newInputStream() (with try-with-resources of course) is really that much preferable to new FileInputStream() then the IDE should have a hint for it. Right?

Must admit this whole thing is news to me. I can see that Stephen Conollly's blog post has gone viral and triggered many projects to review their usage of FileInputStream (or FileOutputStream). His article was later published on DZone were it triggered a minor discussion where Kirk Pepperdine also commented. I don't see Mr Pepperdine disputing that a finalizer causes GC but he seems to argue that it can hardly have that much of an impact. Anyway, that's why it will be interesting to see if there's any real impact on a large code base like NetBeans.

Bottom line: I think we should see some more test results before we begin changing every occurrence of FileInputStream.

Interesting stuff, mdindoffer !

mdindoffer · 2017-11-27T20:47:28Z

@emilianbold Done manually. Sure there may be a place for a java hint in the future, but this was partly an experiment. Also, the code is kinda fragile and tangled up in some places, so I prefer to do this stuff manually.

The idea was to apply this change throughout the whole repo in one go, but it proved to be more time consuming than I thought. Yes, the speedup that seems to be present at the boot probably originates from the core. And maybe autoupdate too? If it checks for updates at startup, not entirely sure. However, the point is, if there's a benefit observable in one use case (boot), it should manifest itself in other use cases too, even if negligibly. I also planned to tackle code that touches file searching & replacing and benchmark that. I think the api.search* module (which I plowed through) houses most of the relevant code for this, but not all. However I am not that much familiar with the NB cluster structure to say that for sure.
"but don't like so much" - so, are you against this FI/OS change in general?

@lbruun
I did not touch the JVM / GC params apart from the Xmx and gc logging. The XXs you list are defaults for x64 Java 8.
The IDE hint seems like a good idea. But as you mentioned, the real life effect can be negligible for most of the use cases nowadays.
I will address the comments and then run some more tests.
BTW this post also provides some nice graphs

emilianbold · 2017-11-27T21:13:46Z

"but don't like so much" - so, are you against this FI/OS change in general?

try-with-resources seems a clear gain because it removes so many finally blocks that just close the stream.

Files.newInputStream just looks more ugly with the extra path creation, StandardOpenOption flags, etc.

AFAIK FileInputStream is not outright deprecated and I'm curious if there's really any kind of noticeable impact during normal IDE usage (editing a source file, running the project, etc).

For startup having the most efficient code is important so it would be interesting to narrow down the core modules that need this upgrade.

Basically I would slice-and-dice this into more commits / PRs and even have places where the old code is left alone (because I'm not convinced it has a noticeable effect and there's really no urgent need to switch to the new API and abandon FileInputStream forever).

This change drops the usage of FileInputStream and FileOutputStream in cluster modules "a*" through "c*". The FIS/FOS are putting unnecesarry pressure on the GC because of their finalize() method. I've measured small, but fairly consistent improvements in GC pauses during the startup of the IDE with couple of projects opened: Before: http://gceasy.io/my-gc-report.jsp?p=c2hhcmVkLzIwMTcvMTEvMjYvLS1nYXJiYWdlLWNvbGxlY3Rpb24tYmVmb3JlLXN0YXJ0dXAubG9nLS0yMC0xMS01OQ== After: http://gceasy.io/my-gc-report.jsp?p=c2hhcmVkLzIwMTcvMTEvMjYvLS1nYXJiYWdlLWNvbGxlY3Rpb24tYWZ0ZXIxY29tbWl0LXN0YXJ0dXAubG9nLS0yMC0xMy01Mw== In every module that required it (for the "Files" class workaround), theres a bump in javac source to version 7. Further references: see JDK-8080225, HDFS-8526, JENKINS-42934 Address comments from emilianbold

emilianbold reviewed Nov 26, 2017

View reviewed changes

mdindoffer force-pushed the dropfisfos branch 3 times, most recently from 8498a40 to 7aa2b0b Compare December 2, 2017 10:41

mdindoffer force-pushed the dropfisfos branch from 7aa2b0b to 1a85004 Compare December 2, 2017 15:09

mdindoffer closed this Dec 10, 2017

mdindoffer deleted the dropfisfos branch December 10, 2017 18:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Drop usage of FileInput/OutputStreams - modules a* -> c* #295

Drop usage of FileInput/OutputStreams - modules a* -> c* #295

mdindoffer commented Nov 26, 2017

emilianbold left a comment •

edited

emilianbold Nov 26, 2017

emilianbold Nov 26, 2017

emilianbold Nov 26, 2017

mdindoffer Nov 27, 2017

emilianbold Nov 26, 2017

emilianbold Nov 26, 2017

lbruun commented Nov 27, 2017

lbruun commented Nov 27, 2017

mdindoffer commented Nov 27, 2017 •

edited

emilianbold commented Nov 27, 2017

Drop usage of FileInput/OutputStreams - modules a* -> c* #295

Drop usage of FileInput/OutputStreams - modules a* -> c* #295

Conversation

mdindoffer commented Nov 26, 2017

emilianbold left a comment • edited

Choose a reason for hiding this comment

emilianbold Nov 26, 2017

Choose a reason for hiding this comment

emilianbold Nov 26, 2017

Choose a reason for hiding this comment

emilianbold Nov 26, 2017

Choose a reason for hiding this comment

mdindoffer Nov 27, 2017

Choose a reason for hiding this comment

emilianbold Nov 26, 2017

Choose a reason for hiding this comment

emilianbold Nov 26, 2017

Choose a reason for hiding this comment

lbruun commented Nov 27, 2017

lbruun commented Nov 27, 2017

mdindoffer commented Nov 27, 2017 • edited

emilianbold commented Nov 27, 2017

emilianbold left a comment •

edited

mdindoffer commented Nov 27, 2017 •

edited