I came across some code in our code base that seems not wrong, but terribly inefficient due the the nature of our platform (a lot of threads could be accessing these two methods simultaneously).

ConcurrentMap<String, Post> postById = new ConcurrentHashMap<>();
ArrayDeque<Post> posts = new Dequeue<>();

synchronized public Post addPost(String postId, Date createdAt, String link, String profileId) {
    Post post = new Post(postId, link, createdAt, profileId);
    if (postsById.containsKey(postId)) {
        postsById.replace(postId, post);
        for (Post removeCandidate: posts) {
            if (removeCandidate.getId().equals(postId)) {
                posts.remove(removeCandidate);
                break;
            }
        }
    }
    if (posts.size() >= maxPostsPerUser) {
        Post removed = posts.removeLast();
        postsById.remove(removed.getId());
    }
    postsById.put(postId, post);
    posts.addFirst(post);   
    
    return post;
}

synchronized public Post addUpdate(String postId, Date createdAt, long numReplies, long numReposts, long numLikes, String link, String profileId) {
    Post post = postsById.get(postId);
    if (post != null) {
        post.setNumReplies(numReplies);
        post.setNumReposts(numReposts);
        post.setNumLikes(numLikes);
    } else {
        post = new Post(postId, link, createdAt, profileId);
        post.setNumReplies(numReplies);
        if (posts.size() >= maxPostsPerUser) {
            Post removed = posts.removeLast();
            postsById.remove(removed.getId());
        }
        postsById.put(postId, post);
        posts.addFirst(post);           
    }
    
    return post;
}

I personally hate synchronized blocks anywhere (yes, sometimes it's the only way, rarely)

But in this scenario I am at a slight loss. I was thinking of locking access on the postId somehow, so contention would be at a much finer grain. Not sure if String.intern() would make this possible. Looking for suggestions.

I came across some code in our code base that seems not wrong, but terribly inefficient due the the nature of our platform (a lot of threads could be accessing these two methods simultaneously).

ConcurrentMap<String, Post> postById = new ConcurrentHashMap<>();
ArrayDeque<Post> posts = new Dequeue<>();

synchronized public Post addPost(String postId, Date createdAt, String link, String profileId) {
    Post post = new Post(postId, link, createdAt, profileId);
    if (postsById.containsKey(postId)) {
        postsById.replace(postId, post);
        for (Post removeCandidate: posts) {
            if (removeCandidate.getId().equals(postId)) {
                posts.remove(removeCandidate);
                break;
            }
        }
    }
    if (posts.size() >= maxPostsPerUser) {
        Post removed = posts.removeLast();
        postsById.remove(removed.getId());
    }
    postsById.put(postId, post);
    posts.addFirst(post);   
    
    return post;
}

synchronized public Post addUpdate(String postId, Date createdAt, long numReplies, long numReposts, long numLikes, String link, String profileId) {
    Post post = postsById.get(postId);
    if (post != null) {
        post.setNumReplies(numReplies);
        post.setNumReposts(numReposts);
        post.setNumLikes(numLikes);
    } else {
        post = new Post(postId, link, createdAt, profileId);
        post.setNumReplies(numReplies);
        if (posts.size() >= maxPostsPerUser) {
            Post removed = posts.removeLast();
            postsById.remove(removed.getId());
        }
        postsById.put(postId, post);
        posts.addFirst(post);           
    }
    
    return post;
}

I personally hate synchronized blocks anywhere (yes, sometimes it's the only way, rarely)

But in this scenario I am at a slight loss. I was thinking of locking access on the postId somehow, so contention would be at a much finer grain. Not sure if String.intern() would make this possible. Looking for suggestions.

• java
• multithreading
• synchronization

Share Improve this question Follow edited Mar 22 at 8:59 Mark Rotteveel 110k229229 gold badges156156 silver badges224224 bronze badges asked Mar 21 at 20:06 GandalfGandalf 9,85588 gold badges5858 silver badges9191 bronze badges 3

• 1 If this is 'working' code the you should ask in codereview.stackexchange. I imagine that it can have some issues with both methods modifying concurrently the ArrayDeque however you didn't mention any specific issues. synchronized will prevent concurrent access to a single method. It would be also important to explain how these methods are used. – aled Commented Mar 21 at 20:48
• @aled synchronized will not just "prevent concurrent access to a single method", it will prevent all concurrent access that synchronizes on the same monitor. – Mark Rotteveel Commented Mar 22 at 9:00
• @MarkRotteveel my mistake, thanks for the clarification. – aled Commented Mar 22 at 17:36

Add a comment  | 

1 Answer 1

Sorted by: Reset to default 0

Came across some code in our code base that seems (not wrong) but terribly inefficient due the the nature of our platform (a lot of threads could be accessing these two methods simultaneously.

Before we get into the details of your problem ... do you have any evidence that this really is inefficient? That it has an actual impact on performance? That the impact is significant?

If not, this could be a case of premature optimization. My advice would be to use a profiler or some other technique to figure out whether there is a real problem here ... before investing lots of time in solving it.

---

Looking at the code, it strikes me that the for loop in addPost is a potential performance bottleneck. Since it is performed while holding the lock, it is a potential concurrency bottleneck as well. So:

• See if there is a way to do the removal from posts more efficiently.
• See if there is a way to do it without holding the lock; e.g. by relaxing the implicit constraints.
• Maybe see if you can avoid the removal entirely; e.g. if the purpose of the posts Deque is to be a work queue, you could check if a post is the latest for a user before processing it. That is an O(1) operation.

Profiling should tell you if the for loop is actually a performance bottleneck. So profile first!