Should I cache System.getProperty(“line.separator”

Question

Consider such method:

@Override
public String toString()
{
    final StringBuilder sb = new StringBuilder();
    for (final Room room : map)
    {
        sb.append(room.toString());
        sb.append(System.getProperty("line.separator")); // THIS IS IMPORTANT
    }
    return sb.toString();
}

System.getProperty("line.separator") can be called many times.

Should I cache this value with public final static String lineSeperator = System.getProperty("line.separator") and later use only lineSeperator?

Or System.getProperty("line.separator") is as fast as using a static field?

Answer 1

I see your question as presenting a false dichotomy. I would neither call getProperty every time, nor declare a static field for it. I'd simply extract it to a local variable in toString.

@Override
public String toString()
{
    final StringBuilder sb = new StringBuilder();
    final String newline = System.getProperty("line.separator"); 
    for (final Room room : map) sb.append(room.toString()).append(newline);
    return sb.toString();
}

BTW I have benchmarked the call. The code:

public class GetProperty
{
  static char[] ary = new char[1];
  @GenerateMicroBenchmark public void everyTime() {
    for (int i = 0; i < 100_000; i++) ary[0] = System.getProperty("line.separator").charAt(0);
  }
  @GenerateMicroBenchmark public void cache() {
    final char c = System.getProperty("line.separator").charAt(0);
    for (int i = 0; i < 100_000; i++) ary[0] = (char)(c | ary[0]);
  }
}

The results:

Benchmark                     Mode Thr    Cnt  Sec         Mean   Mean error    Units
GetProperty.cache            thrpt   1      3    5       10.318        0.223 ops/msec
GetProperty.everyTime        thrpt   1      3    5        0.055        0.000 ops/msec

The cached approach is more than two orders of magnitude faster.

Do note that the overall impact of getProperty call against all that string building is very, very unlikely to be noticeable.

Answer 2

You do not need to fear that the line separator will change while your code is running, so I see no reason against caching it.

Caching a value is certainly faster than executing a call over and over, but the difference will probably be negligible.

Answer 3

Since it's so easy to do, why not? At the very least the implementation of System.getProperty() will have to do a hash table lookup (even if cached internally) to find the property you are requesting, then the virtual method getString() will be called on the resulting Object. None of these are very expensive but will need to be called multiple times. Not to mention many String temporaries will be created and need GCing after.

If you move this out to the top of your loop and reuse the same value, you avoid all of these problems. So why not?

Answer 4

If the system property is guaranteed to remain constant during the application it can be cached but in general you will loose the feature of the property which is changing the behavior when you change it.

For instance a text generator could use the property to generate text for windows or for linux and allow the property to be changed dynamically in the application, why not ?

In general, catching a property implies making useless the function setProperty.

Answer 5

If you have become aware of a performance problem that you know relates to this, yes.

If you haven't, then no, the lookup is unlikely to have enough overhead to matter.

This would fall under either or both of the general categories "micro-optimization" and "premature optimization." :-)

---

But if you're worried about efficiency, you probably have a much bigger opportunity in that your toString method is regenerating the string every time. If toString will be called a lot, rather than caching the line terminator, cache the generated string, and clear that whenever your map of rooms changes. E.g.:

@Override
public String toString()
{
    if (cachedString == null)
    {
        final StringBuilder sb = new StringBuilder();
        final String ls = System.getProperty("line.separator");
        for (final Room room : map)
        {
            sb.append(room.toString());
            sb.append(ls);
        }
        cachedString = sb.toString();
    }
    return cachedString;
}

...and when your map changes, do

cachedString = null;

That's a lot more bang for the buck (the buck being the overhead of an extra field). Granted it's per-instance rather than per-class, so (reference earlier comment about efficiency) only do it if you have a good reason to.