587884 - Optimize PrintWriter print() for String class

Status: RESOLVED FIXED

(Tamarin Graveyard :: Virtual Machine, defect)

Description

[Rick Reitmaier]

Attachment: add writeN()

String::print(PrintWriter& p) calls p.writeN() which copies the string contents into a buffer, null terminates it and then calls write() on it.

StringOutputStream then calls writeN() computing a length for the string and again copies it into a final buffer.

This whole sequence could be optimized by allowing OutputStream a writeN() method, avoiding the copying and multiple length computations.

Comment 1

[Edwin Smith]

Comment on attachment 466500 [details] [diff] [review]
add writeN()

nit: please preserve exiting comment formatting that uses // instead of /* */

Since the new ConsoleOutputStream field m_buffer (a StringBuffer) is only used transiently, must it be a member?

Also, I'm suspicious about it being a GCObject member of annother GCObject -- will its internal write barriers find the start of the outermost GCObject?

(not this bug, but the whole OutputStream class heirarchy seems rife with GC pain).

Comment 2

[Edwin Smith]

Comment on attachment 466500 [details] [diff] [review]
add writeN()

R- because of the m_buffer declaration; StringBuffer extends GCObject so it can't be a member of another GCObject.  (existing cases of this are considered bugs).

Comment 3

[Rick Reitmaier]

Attachment: ver 2

Addressed comments.  The StringBuffer is kept around since it will be used throughout the lifetime of the ConsoleOutputStream.   We could keep building it each call to writeN() but that seems wasteful.

Comment 4

[Edwin Smith]

Comment on attachment 467609 [details] [diff] [review]
ver 2

ConsoleOutputStream:53  I'm sorry to R- again, but in avoiding MMgc pitfall #27-part-b, you managed to fall into MMgc pitfall #1: missing write barrier.  You need a WB() around the assignment, or a DWB() around the StringBuffer* declaration.

Could you punctuate and clarify the comment "careful with this it usually results in a copy by the stream" -- what does "usually" mean?  what does being careful entail?

It looks like the new buffer member will grow to be as big as the largest single stream we ever print, and never shrink.  That ok?

Comment 5

[Rick Reitmaier]

Attachment: ver3

Three times lucky?

Big oops; added DWB()

>> It looks like the new buffer member will grow to be as big as the largest
single stream we ever print, and never shrink.  That ok?

I believe it should be fine as it will only grow as large as 'count' of a single call to writeN().  After logMessage(), m_buffer->reset() is called which prevents unbounded growth.

Comment 6

[Edwin Smith]

Comment on attachment 467822 [details] [diff] [review]
ver3

Much better comment, thank you!

Comment 7

[Rick Reitmaier]

http://hg.mozilla.org/tamarin-redux/rev/679d167ae77f