Why the overall performance of application degrades if I do null checks on the maps?

Question

Below is my class which uses CountDownLatch to make sure reads are not happening on the primary, secondary and tertiary maps for the first time whenever writes are happening on those maps.

public class ClientData {

    public static class Mappings {
        public final Map<String, Map<Integer, String>> primary;
        public final Map<String, Map<Integer, String>> secondary;
        public final Map<String, Map<Integer, String>> tertiary;

        public Mappings(
            Map<String, Map<Integer, String>> primary,
            Map<String, Map<Integer, String>> secondary,
            Map<String, Map<Integer, String>> tertiary
        ) {
            this.primary = primary;
            this.secondary = secondary;
            this.tertiary = tertiary;
        }
    }

    private static final AtomicReference<Mappings> mappings = new AtomicReference<>();
    private static final CountDownLatch hasBeenInitialized = new CountDownLatch(1);

    public static Mappings getMappings() {
        try {
            hasBeenInitialized.await();
            return mappings.get();
        } catch (InterruptedException e) {
            Thread.currentThread().interrupt();
            throw new IllegalStateException(e);
        }
    }

    public static void setMappings(
        Map<String, Map<Integer, String>> primary,
        Map<String, Map<Integer, String>> secondary,
        Map<String, Map<Integer, String>> tertiary
    ) {
        setMappings(new Mappings(primary, secondary, tertiary));
    }

    public static void setMappings(Mappings newMappings) {
        mappings.set(newMappings);
        hasBeenInitialized.countDown();
    }
}

And below is my background thread class which is only responsible for setting all the three maps (look for parseResponse method below). It runs every 10 minutes.

public class TempBackgroundThread {

    // parse the response and store it in a variable
    private void parseResponse(String response) {
        //...       
        Map<String, Map<Integer, String>> primaryTables = null;
        Map<String, Map<Integer, String>> secondaryTables = null;
        Map<String, Map<Integer, String>> tertiaryTables = null;

        //...

        // store the three map data in ClientData class variables if anything has changed  
        // which can be used by other threads, this will be updated once every four or five months
        if(changed) {
            ClientData.setMappings(primaryTables, secondaryTables, tertiaryTables);
        }
    }
}

Problem Statement:

If I am doing all sort of null or sanity checks on my mappings object and primary, secondary and tertiary maps, peformance degrades a lot (not sure why). But if I don't do any sanity or null checks, performance comes very good. Can anyone explain me what's wrong and why it happens?

Below is an example -

I am using ClientData class to get all the mappings in my main thread. As you can see below, I am doing all sort of sanity checks to make sure mappings, mappings.primary, mappings.secondary and mappings.tertiary are not empty. If they are empty, then log an error and return

class Task implements Callable<String> {

    public Task() {
    }

    public String call() throws Exception {

        int compId = 100;
        String localPath = "hello";
        String remotePath = "world";

        Mappings mappings = ClientData.getMappings(); 

        if (MyUtilityClass.isEmpty(mappings)
                || (MyUtilityClass.isEmpty(mappings.primary) && MyUtilityClass
                        .isEmpty(mappings.secondary))
                || MyUtilityClass.isEmpty(mappings.tertiary)) {

            // log error and return
        }

        // otherwise extract values from them
        String localPAddress = null;
        String remotePAddress = null;
        if (MyUtilityClass.isNotEmpty(mappings.primary)) {
            String localPId = mappings.primary.get(localPath).get(compId);
            localPAddress = mappings.tertiary.get(localPath).get(
                    Integer.parseInt(localPId));
            String remotePId = mappings.primary.get(remotePath).get(compId);
            remotePAddress = mappings.tertiary.get(remotePath).get(
                    Integer.parseInt(remotePId));
        }

        String localSAddress = null;
        String remoteSAddress = null;
        if (MyUtilityClass.isNotEmpty(mappings.secondary)) {
            String localSId = mappings.secondary.get(localPath).get(compId);
            localSAddress = mappings.tertiary.get(localPath).get(
                    Integer.parseInt(localSId));
            String remoteSId = mappings.secondary.get(remotePath).get(compId);
            remoteSAddress = mappings.tertiary.get(remotePath).get(
                    Integer.parseInt(remoteSId));
        }

        // now use - localPAddress, remotePAddress, localSAddress and remoteSAddress
    }
}

With the above sanity and null checks on primary, secondary and tertiary mappings, overall performance (95th percentile) of application comes as 4 ms.

But if I do it like this without any sanity checks or null checks on primary, secondary and tertiary mappings, I get overall perforamnce (95th percentile) as 0.87 ms.

class Task implements Callable<String> {

    public Task() {
    }

    public String call() throws Exception {

        int compId = 100;
        String localPath = "hello";
        String remotePath = "world";

        Mappings mappings = ClientData.getMappings(); 

        String localPId = mappings.primary.get(localPath).get(compId);
        String localPAddress = mappings.tertiary.get(localPath).get(Integer.parseInt(localPId));
        String remotePId = mappings.primary.get(remotePath).get(compId);
        String remotePAddress = mappings.tertiary.get(remotePath).get(Integer.parseInt(remotePId));
        String localSId = mappings.secondary.get(localPath).get(compId);
        String localSAddress = mappings.tertiary.get(localPath).get(Integer.parseInt(localSId));
        String remoteSId = mappings.secondary.get(remotePath).get(compId);
        String remoteSAddress = mappings.tertiary.get(remotePath).get(Integer.parseInt(remoteSId));

        // now use - localPAddress, remotePAddress, localSAddress and remoteSAddress
    }
}

Below is my isEmpty and isNotEmpty method -

public static boolean isNotEmpty(Object obj) {
    return !isEmpty(obj);
}

public static boolean isEmpty(Object obj) {
    if (obj == null)
        return true;
    if (obj instanceof Collection)
        return ((Collection<?>) obj).size() == 0;

    final String s = String.valueOf(obj).trim();

    return s.length() == 0 || s.equalsIgnoreCase("null");
}

This is so much information. I wish both the information and the code were boiled down to its essentials. — aliteralmind, Jul 05 '14 at 21:47
@aliteralmind Thanks for feedback. Let me try to shorten it down. — john, Jul 05 '14 at 21:48
@aliteralmind I have updated the question by removing the stuff which I can remove to make it short and understandable. — john, Jul 05 '14 at 21:55

score 1 · Accepted Answer · answered Jul 05 '14 at 22:01

1

See how often your code gets to this point. This can be expensive with some complex objects and their heavy #toString() methods:

final String s = String.valueOf(obj).trim();

Also it creates temporary garbage that might cause Garbage Collection while your test is counting.

answered Jul 05 '14 at 22:01

Alex Malko

126
1
4

Thanks for suggestion. Do you have any suggestion on how my isEmpty method should look like that can cover all the cases? I can try that out now and see whether it improves something or not? – john Jul 05 '14 at 22:03
I did not follow your long question in details. But I'd suggest you to cover only those possible types that can come to "isEmpty()". For that purpose you can put this as first line in "isEmpty()": System.out.println(obj.getClass().getName()); Then implement the "isEmpty()" only for those types. – Alex Malko Jul 05 '14 at 22:08

score 0 · Answer 2 · answered Jul 05 '14 at 22:17

Your sanity checks ensure that no code is executed unless everything is perfect. If any of the checks fail, you log and then return. Only if the sanity checks succeed, then you proceed.

Instead, consider proceeding blindly, on the assumption that everything is fine. But surround each critical pieces with a try-catch. In the catches, you then check for the specific error, only for the sake of an accurate exception message.

For example, never do this:

public void getStringLength(String string_thatShouldNeverBeNull)  {
   Objects.requireNonNull(string_thatShouldNeverBeNull, "string_thatShouldNeverBeNull");
   return  string_thatShouldNeverBeNull.length();
}

When you can do this instead:

public void getStringLength(String string_thatShouldNeverBeNull)  {
   try  {
      return  string_thatShouldNeverBeNull.length();
   }  catch(NullPointerException npx)  {
      throw  new NullPointerException("string_thatShouldNeverBeNull");
   }      
}

The reason is that the output/error-response is always the same for both of these functions, whether the parameter is null or not. So why do the extra check when it is likely to be valid most of the time? It's wasteful.

There are some situations where this not possible. An example:

public void setTheString(String string_thatShouldNeverBeNull)  {
   Objects.requireNonNull(string_thatShouldNeverBeNull, "string_thatShouldNeverBeNull");
   str = string_thatShouldNeverBeNull;
}

There's never an opportunity for the error to be thrown, so the check must be done.

Why the overall performance of application degrades if I do null checks on the maps?

2 Answers2

Linked