-
Notifications
You must be signed in to change notification settings - Fork 24
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.
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
HTTPServerRequest: Build urlURL using URLComponents #143
Conversation
@nethraravindran Bridging UInt16 to NSNumber doesn't work pre-4.2. We have two options:
I ran a small benchmark and find performance of |
c9b75e2
to
02a8da3
Compare
@pushkarnk I have addressed the changes. Please review. Thank you! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Performance apart, this seems like a better way of building urlURL
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have no problem with this in principle, but we are creating a NSURLComponents()
and throwing it away. We also have this property (line 43):
public var urlComponents: URLComponents
so could we cache the URLComponents
we create and use it in that property? This all seems a bit messy at the moment.
02a8da3
to
1079b8b
Compare
Codecov Report
@@ Coverage Diff @@
## master #143 +/- ##
==========================================
- Coverage 67.11% 66.95% -0.16%
==========================================
Files 20 20
Lines 1271 1274 +3
==========================================
Hits 853 853
- Misses 418 421 +3
Continue to review full report at Codecov.
|
1079b8b
to
9a72d2d
Compare
if let _urlComponents = _urlComponents { | ||
return _urlComponents | ||
} else { | ||
return URLComponents(url: urlURL, resolvingAgainstBaseURL: false) ?? URLComponents() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nethraravindran We must cache it here as well, in the scenario where urlComponents
was invoked before urlURL
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pushkarnk Sure!
|
||
self.enableSSL ? url.append("https://") : url.append("http://") | ||
_urlComponents = URLComponents() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
check if _urlComponents
already exists here, we could have initialised it in urlComponents
9a72d2d
to
46fbee0
Compare
This increases the performance by ~2% while accessing
urlURL