Skip to content

feature/regex-pre-compile #91

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Dec 1, 2020
Merged

feature/regex-pre-compile #91

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 11 additions & 2 deletions src/main/java/com/microsoft/graph/content/MSBatchRequestContent.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@
import java.util.List;
import java.util.Map;
import java.util.concurrent.ThreadLocalRandom;
import java.util.regex.Matcher;
import java.util.regex.Pattern;

import com.google.gson.JsonArray;
import com.google.gson.JsonElement;
Expand Down Expand Up @@ -113,12 +115,19 @@ public String getBatchRequestContent() {
return content;
}

private static final Pattern protocolAndHostReplacementPattern = Pattern.compile("(?i)^http[s]?:\\/\\/graph\\.microsoft\\.com\\/(?>v1\\.0|beta)\\/?"); // (?i) case insensitive
private Matcher protocolAndHostReplacementMatcher; // not static to avoid multithreading issues as the object is mutable
private JsonObject getBatchRequestObjectFromRequestStep(final MSBatchRequestStep batchRequestStep) {
final JsonObject contentmap = new JsonObject();
contentmap.add("id", new JsonPrimitive(batchRequestStep.getRequestId()));

final String url = batchRequestStep.getRequest().url().toString()
.replaceAll("(?i)^http[s]?:\\/\\/graph\\.microsoft\\.com\\/(?>v1\\.0|beta)\\/?", ""); // (?i) case insensitive
if(protocolAndHostReplacementMatcher == null) {
protocolAndHostReplacementMatcher = protocolAndHostReplacementPattern.matcher(batchRequestStep.getRequest().url().toString());
} else {
protocolAndHostReplacementMatcher = protocolAndHostReplacementMatcher.reset(batchRequestStep.getRequest().url().toString());
}
Comment on lines +119 to +128
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't safe to do since getBatchRequestObjectFromRequestStep is called by getBatchRequestContent, which is a public method, and therefore can be called concurrently by multiple threads. You can do the following:

  1. synchronize the method
  2. use a thread local Matcher
  3. just do the Pattern.compile as a static and allocate a new Matcher each time in the loop
  4. pass in a a Matcher object from getBatchRequestContent that you initialize outside of the for loop

I would do 3 or 4 personally. I don't think allocating a matcher object is very expensive it's really the regex compilation that you want to avoid doing over and over.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I'm saying this after it's been merged... I've been very busy at work the past few weeks and haven't been able to watch github like I normally do during the day.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No worries, I should have waited for your review before merging since you made the original comment, my bad.
I didn't know that java could do multithreading on a simple for loop. Or are you saying that callers could use the class in a multi-threaded context?
I've submitted #93 for you to review, according to this the change would represent a 50% performance drop. But that's already on 4 times less replaceAll and a pre-compilation that represents (with a big extrapolation) a 1400% improvement over the original code.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I meant the caller could use the class in a multi-threaded context. If there was a guarantee that getBatchRequestContent could never be called simultaneously it would be safe. Your new change looked good :)

If this were a very hot loop it'd probably make sense to go with option 3 (from my previous comment) but this really shouldn't be called that often where iit will really matter.


final String url = protocolAndHostReplacementMatcher.replaceAll("");
contentmap.add("url", new JsonPrimitive(url));

contentmap.add("method", new JsonPrimitive(batchRequestStep.getRequest().method().toString()));
Expand Down