From 44552da606048720de11f89321dc767ef291c391 Mon Sep 17 00:00:00 2001 From: Andrew Stadler Date: Tue, 9 Mar 2010 20:30:47 -0800 Subject: [PATCH] Reduce memory overhead in Message and MimeMessage Message and MimeMessage were creating a lot of unnecessary sub-objects even when not needed, so do a bunch of lazy initialization. This should raise the bar on the size of gigantic inboxes giving us trouble. * Specific optimizations: * Replace date formatter with a shared static * lazy create mHeader (ArrayList) * lazy create mFlags (HashSet) * optimize MimeHeader fields class * lazy create local message-ID (expensive-to-make uuid String) * make message-id string less expensive to create * Other cleanups: * add some override annotations * privatize some members * update a fragile test (not a deep fix, it's still fragile) Side effect, should be faster too. Bug: 2357564 Bug: 2093422 Change-Id: I8a873879d402e2662339d5398ad0b15da6e580e9 --- src/com/android/email/mail/Message.java | 21 ++++- .../email/mail/internet/MimeHeader.java | 7 +- .../email/mail/internet/MimeMessage.java | 93 +++++++++++-------- .../android/email/mail/store/ImapStore.java | 6 +- .../android/email/mail/store/LocalStore.java | 1 + .../email/mail/internet/MimeMessageTest.java | 6 +- 6 files changed, 80 insertions(+), 54 deletions(-) diff --git a/src/com/android/email/mail/Message.java b/src/com/android/email/mail/Message.java index b52e6c97d..8906caf30 100644 --- a/src/com/android/email/mail/Message.java +++ b/src/com/android/email/mail/Message.java @@ -26,7 +26,7 @@ public abstract class Message implements Part, Body { protected String mUid; - protected HashSet mFlags = new HashSet(); + private HashSet mFlags = null; protected Date mInternalDate; @@ -93,24 +93,35 @@ public abstract class Message implements Part, Body { public abstract void removeHeader(String name) throws MessagingException; + // Always use these instead of getHeader("Message-ID") or setHeader("Message-ID"); + public abstract void setMessageId(String messageId) throws MessagingException; + public abstract String getMessageId() throws MessagingException; + public abstract void setBody(Body body) throws MessagingException; public boolean isMimeType(String mimeType) throws MessagingException { return getContentType().startsWith(mimeType); } + private HashSet getFlagSet() { + if (mFlags == null) { + mFlags = new HashSet(); + } + return mFlags; + } + /* * TODO Refactor Flags at some point to be able to store user defined flags. */ public Flag[] getFlags() { - return mFlags.toArray(new Flag[] {}); + return getFlagSet().toArray(new Flag[] {}); } public void setFlag(Flag flag, boolean set) throws MessagingException { if (set) { - mFlags.add(flag); + getFlagSet().add(flag); } else { - mFlags.remove(flag); + getFlagSet().remove(flag); } } @@ -126,7 +137,7 @@ public abstract class Message implements Part, Body { } public boolean isSet(Flag flag) { - return mFlags.contains(flag); + return getFlagSet().contains(flag); } public abstract void saveChanges() throws MessagingException; diff --git a/src/com/android/email/mail/internet/MimeHeader.java b/src/com/android/email/mail/internet/MimeHeader.java index 8278264ae..5257c1b7b 100644 --- a/src/com/android/email/mail/internet/MimeHeader.java +++ b/src/com/android/email/mail/internet/MimeHeader.java @@ -131,10 +131,9 @@ public class MimeHeader { writer.flush(); } - class Field { - String name; - - String value; + private static class Field { + final String name; + final String value; public Field(String name, String value) { this.name = name; diff --git a/src/com/android/email/mail/internet/MimeMessage.java b/src/com/android/email/mail/internet/MimeMessage.java index 67a2313fe..7978f303d 100644 --- a/src/com/android/email/mail/internet/MimeMessage.java +++ b/src/com/android/email/mail/internet/MimeMessage.java @@ -33,7 +33,6 @@ import org.apache.james.mime4j.field.Field; import android.text.TextUtils; import java.io.BufferedWriter; -import java.io.ByteArrayOutputStream; import java.io.IOException; import java.io.InputStream; import java.io.OutputStream; @@ -49,28 +48,31 @@ import java.util.regex.Pattern; * RFC 2045 style headers. */ public class MimeMessage extends Message { - protected MimeHeader mHeader = new MimeHeader(); - protected MimeHeader mExtendedHeader; + private MimeHeader mHeader; + private MimeHeader mExtendedHeader; // NOTE: The fields here are transcribed out of headers, and values stored here will supercede // the values found in the headers. Use caution to prevent any out-of-phase errors. In // particular, any adds/changes/deletes here must be echoed by changes in the parse() function. - protected Address[] mFrom; - protected Address[] mTo; - protected Address[] mCc; - protected Address[] mBcc; - protected Address[] mReplyTo; - protected Date mSentDate; + private Address[] mFrom; + private Address[] mTo; + private Address[] mCc; + private Address[] mBcc; + private Address[] mReplyTo; + private Date mSentDate; + private Body mBody; + protected int mSize; + + // Shared random source for generating local message-id values + private static java.util.Random sRandom = new java.util.Random(); // In MIME, en_US-like date format should be used. In other words "MMM" should be encoded to // "Jan", not the other localized format like "Ene" (meaning January in locale es). // This conversion is used when generating outgoing MIME messages. Incoming MIME date // headers are parsed by org.apache.james.mime4j.field.DateTimeField which does not have any // localization code. - protected SimpleDateFormat mDateFormat = + private static final SimpleDateFormat DATE_FORMAT = new SimpleDateFormat("EEE, dd MMM yyyy HH:mm:ss Z", Locale.US); - protected Body mBody; - protected int mSize; // regex that matches content id surrounded by "<>" optionally. private static final Pattern REMOVE_OPTIONAL_BRACKETS = Pattern.compile("^]+)>?$"); @@ -78,24 +80,22 @@ public class MimeMessage extends Message { private static final Pattern END_OF_LINE = Pattern.compile("\r?\n"); public MimeMessage() { - /* - * Every new messages gets a Message-ID - */ - try { - // TODO: This is wasteful, since we overwrite it on incoming or locally-read messages. - // Should only generate it on as-needed basis. - setMessageId(generateMessageId()); - } - catch (MessagingException me) { - throw new RuntimeException("Unable to create MimeMessage", me); - } + mHeader = null; } + /** + * Generate a local message id. This is only used when none has been assigned, and is + * installed lazily. Any remote (typically server-assigned) message id takes precedence. + * @return a long, locally-generated message-ID value + */ private String generateMessageId() { StringBuffer sb = new StringBuffer(); sb.append("<"); for (int i = 0; i < 24; i++) { - sb.append(Integer.toString((int)(Math.random() * 35), 36)); + // We'll use a 5-bit range (0..31) + int value = sRandom.nextInt() & 31; + char c = "0123456789abcdefghijklmnopqrstuv".charAt(value); + sb.append(c); } sb.append("."); sb.append(Long.toString(System.currentTimeMillis())); @@ -117,7 +117,7 @@ public class MimeMessage extends Message { protected void parse(InputStream in) throws IOException, MessagingException { // Before parsing the input stream, clear all local fields that may be superceded by // the new incoming message. - mHeader.clear(); + getMimeHeaders().clear(); mFrom = null; mTo = null; mCc = null; @@ -131,6 +131,17 @@ public class MimeMessage extends Message { parser.parse(new EOLConvertingInputStream(in)); } + /** + * Return the internal mHeader value, with very lazy initialization. + * The goal is to save memory by not creating the headers until needed. + */ + private MimeHeader getMimeHeaders() { + if (mHeader == null) { + mHeader = new MimeHeader(); + } + return mHeader; + } + public Date getReceivedDate() throws MessagingException { return null; } @@ -149,7 +160,7 @@ public class MimeMessage extends Message { } public void setSentDate(Date sentDate) throws MessagingException { - setHeader("Date", mDateFormat.format(sentDate)); + setHeader("Date", DATE_FORMAT.format(sentDate)); this.mSentDate = sentDate; } @@ -305,23 +316,25 @@ public class MimeMessage extends Message { * @param messageId the new Message-ID value * @throws MessagingException */ + @Override public void setMessageId(String messageId) throws MessagingException { setHeader("Message-ID", messageId); } /** - * Get the mime "Message-ID" header. Note, this field is preset (randomly) in every new - * message, so it should never return null. + * Get the mime "Message-ID" header. This value will be preloaded with a locally-generated + * random ID, if the value has not previously been set. * @return the Message-ID header string * @throws MessagingException */ + @Override public String getMessageId() throws MessagingException { - String[] headers = getHeader("Message-ID"); - if (headers != null) { - // There should really only be one Message-ID here - return headers[0]; + String messageId = getFirstHeader("Message-ID"); + if (messageId == null) { + messageId = generateMessageId(); + setMessageId(messageId); } - throw new MessagingException("A message was found without a Message-ID header"); + return messageId; } public void saveChanges() throws MessagingException { @@ -348,23 +361,23 @@ public class MimeMessage extends Message { } protected String getFirstHeader(String name) throws MessagingException { - return mHeader.getFirstHeader(name); + return getMimeHeaders().getFirstHeader(name); } public void addHeader(String name, String value) throws MessagingException { - mHeader.addHeader(name, value); + getMimeHeaders().addHeader(name, value); } public void setHeader(String name, String value) throws MessagingException { - mHeader.setHeader(name, value); + getMimeHeaders().setHeader(name, value); } public String[] getHeader(String name) throws MessagingException { - return mHeader.getHeader(name); + return getMimeHeaders().getHeader(name); } public void removeHeader(String name) throws MessagingException { - mHeader.removeHeader(name); + getMimeHeaders().removeHeader(name); } /** @@ -443,7 +456,9 @@ public class MimeMessage extends Message { */ public void writeTo(OutputStream out) throws IOException, MessagingException { BufferedWriter writer = new BufferedWriter(new OutputStreamWriter(out), 1024); - mHeader.writeTo(out); + // Force creation of local message-id + getMessageId(); + getMimeHeaders().writeTo(out); // mExtendedHeader will not be write out to external output stream, // because it is intended to internal use. writer.write("\r\n"); diff --git a/src/com/android/email/mail/store/ImapStore.java b/src/com/android/email/mail/store/ImapStore.java index c8250ab4a..f01bc708d 100644 --- a/src/com/android/email/mail/store/ImapStore.java +++ b/src/com/android/email/mail/store/ImapStore.java @@ -1198,11 +1198,10 @@ public class ImapStore extends Store { * Message-ID header. If there are more than one response, take the * last one, as it's most likely he newest (the one we just uploaded). */ - String[] messageIdHeader = message.getHeader("Message-ID"); - if (messageIdHeader == null || messageIdHeader.length == 0) { + String messageId = message.getMessageId(); + if (messageId == null || messageId.length() == 0) { continue; } - String messageId = messageIdHeader[0]; List responses = mConnection.executeSimpleCommand( String.format("UID SEARCH (HEADER MESSAGE-ID %s)", messageId)); @@ -1230,6 +1229,7 @@ public class ImapStore extends Store { return null; } + @Override public void setFlags(Message[] messages, Flag[] flags, boolean value) throws MessagingException { checkOpen(); diff --git a/src/com/android/email/mail/store/LocalStore.java b/src/com/android/email/mail/store/LocalStore.java index 122b9dc05..49ae0e5dc 100644 --- a/src/com/android/email/mail/store/LocalStore.java +++ b/src/com/android/email/mail/store/LocalStore.java @@ -1651,6 +1651,7 @@ public class LocalStore extends Store implements PersistentDataCallbacks { return mId; } + @Override public void setFlag(Flag flag, boolean set) throws MessagingException { if (flag == Flag.DELETED && set) { /* diff --git a/tests/src/com/android/email/mail/internet/MimeMessageTest.java b/tests/src/com/android/email/mail/internet/MimeMessageTest.java index f5dd98874..1c72646c2 100644 --- a/tests/src/com/android/email/mail/internet/MimeMessageTest.java +++ b/tests/src/com/android/email/mail/internet/MimeMessageTest.java @@ -61,8 +61,6 @@ public class MimeMessageTest extends TestCase { private final String LONG_PLAIN_256 = LONG_PLAIN_64 + LONG_PLAIN_64 + LONG_PLAIN_64 + LONG_PLAIN_64; - // TODO: more tests. - /** * Confirms that setSentDate() correctly set the "Date" header of a Mime message. * @@ -397,6 +395,7 @@ public class MimeMessageTest extends TestCase { /* * Test for writeTo(), only for header part. + * NOTE: This test is fragile because it assumes headers will be written in a specific order */ public void testWriteToHeader() throws Exception { MimeMessage message = new MimeMessage(); @@ -410,9 +409,10 @@ public class MimeMessageTest extends TestCase { ByteArrayOutputStream out = new ByteArrayOutputStream(); message.writeTo(out); out.close(); - String expectedString = "Message-ID: " + message.getMessageId() + "\r\n" + + String expectedString = "Header1: value1\r\n" + "Header4: value4\r\n" + + "Message-ID: " + message.getMessageId() + "\r\n" + "\r\n"; byte[] expected = expectedString.getBytes(); byte[] actual = out.toByteArray();