Skip to content
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

[TIMOB-25941] Android: Optimized HTTPClient download performance #10274

Merged
merged 4 commits into from Aug 24, 2018

Conversation

jquick-axway
Copy link
Contributor

JIRA:
https://jira.appcelerator.org/browse/TIMOB-25941

Summary:

  • Download performance improved by about 25%. (Depends on network speed/quality.)
  • Increased response buffer size from 4kb to 8kb.
  • Removed undocumented event property blob from callback ondatastream().
    • Improves performance by avoiding extra buffer copy.
    • This undocumented property does not exist on iOS.
  • Modified responseText property handling only create its string when accessed.
    • Was creating and transcoding the string for every response segment received. Was a huge performance hit.
    • Fixed responseText to provide total text downloaded like iOS instead of the buffered response segment's text.
  • Synchronized file write to storage upon download completion.
    • Avoids race condition where reading responseText or responseData in onload() callback may provide an incomplete response.

Test:
@lokeshchdhry, I recommend that you test this at home. The office network is too congested and download speed varies wildly.

  1. Acquire an Android device with Internet access.
  2. Build and run the below code with Titanium 7.3.0.
  3. Tap the "Download" button.
  4. Wait for the download to complete.
  5. Note down the download duration time. (You may want to do this a few times and record the average.)
  6. Build and run with this PR and repeat steps 3-5.
  7. The download time should have improved.
var url = "https://github.com/appcelerator-modules/ti.admob/releases/download/ios-2.2.0/ti.admob-iphone-2.2.0.zip";
var downloadFilePath = Ti.Filesystem.applicationDataDirectory + "downloaded.zip";

var window = Ti.UI.createWindow();
var button = Ti.UI.createButton({ title: "Download" });
var progressBar = Ti.UI.createProgressBar({
	min: 0,
	max: 100,
	bottom: "40dp",
	left: "5dp",
	right: "5dp",
});
button.addEventListener("click", function(e) {
	var startTime = new Date();
	var httpClient = Ti.Network.createHTTPClient({
		ondatastream: function(e) {
			progressBar.value = e.progress * 100;
		},
		onload: function(e) {
			var duration =  (new Date()) - startTime;
			Ti.API.info("@@@ onload() duration: " + duration + "ms");
			alert("Download Time: " + duration + "ms");
			progressBar.value = 100;
			button.enabled = true;
		},
		onerror: function(e) {
			Ti.API.info("@@@ onerror()");
			alert("Download Failed");
			button.enabled = true;
		},
	});
	progressBar.value = 0;
	button.enabled = false;
	httpClient.open("GET", url);
	httpClient.setRequestHeader("Accept-Encoding", "identity");
	httpClient.file = downloadFilePath;
	httpClient.send();
});
window.add(button);
window.add(progressBar);
window.open();

- Increased network response buffer size from 4kb to 8kb.
- Removed "ondatastream" event's undocumented "blob" property.
  * Improves performance by avoiding an extra buffer copy.
  * Note that iOS does not support this undocumented API.
- Modified "responseText" property to only create a string when accessed.
  * Major performance improvement. No longer transcodes per response buffer.
  * Fixed "responseText" to provide total text downloaded like iOS instead of only the buffered segment's text.
System.arraycopy(data, 0, blobData, 0, size);

TiBlob blob = TiBlob.blobFromData(blobData, contentType);
callbackData.put("blob", blob);
Copy link
Contributor

Choose a reason for hiding this comment

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

Technically this is a breaking Api change (and should go into 8.0.0) since this is removed. Although, it's hard to see how many people would be using this undocumented event callback property...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is. But from searching our repo's and JIRA's history, it doesn't appear to be used. We've discussed it internally in the office and decided to remove it.

Also note that we have a customer that wants a solution to this performance issue before Titanium 8.0.0. You may want to talk to Eric about this.

@@ -245,7 +246,7 @@ private void handleResponse(HttpURLConnection connection) throws IOException
Log.d(TAG, "Content length: " + contentLength, Log.DEBUG_MODE);
int count = 0;
long totalSize = 0;
byte[] buf = new byte[4096];
byte[] buf = new byte[8192];
Copy link
Contributor

Choose a reason for hiding this comment

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

Extract the magic constant as a final static at top so it's easier to tweak the buffer size?

(in fact looking at this file it looks like we have a number of magic constants around buffers/sizes: 512 bytes for progress notifications/entity data buffer; 8MB for blob/file I/O buffer; 512Kb for dumping response to file first rather than keep in-memory)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not worth changing to a constant if it's only used in 1 place in the code. It's better to keep it simple.

Yes, there are other place in this class where there is duplication, but that's a refactoring for another day.

@lokeshchdhry
Copy link
Contributor

FR Passed.

An improved performance is seen as compared to 7.3.0 SDK.

Studio Ver: 5.1.0.201808080937
SDK Ver: 7.5.0 local build
OS Ver: 10.13.5
Xcode Ver: Xcode 9.4.1
Appc NPM: 4.2.13
Appc CLI: 7.0.6-master.12
Daemon Ver: 1.1.3
Ti CLI Ver: 5.1.1
Alloy Ver: 1.13.2
Node Ver: 8.9.1
NPM Ver: 5.5.1
Java Ver: 10.0.2
Devices: ⇨ google Nexus 5 (Android 6.0.1)
⇨ google Nexus 6P (Android 8.1.0)

@build
Copy link
Contributor

build commented Aug 24, 2018

Messages
📖

💾 Here's the generated SDK zipfile.

Generated by 🚫 dangerJS

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants