<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=iso-8859-1">
<style type="text/css" style="display:none;"><!-- P {margin-top:0;margin-bottom:0;} --></style>
</head>
<body dir="ltr">
<div id="divtagdefaultwrapper" style="font-size:12pt;color:#000000;font-family:Calibri,Helvetica,sans-serif;" dir="ltr">
<p><span style="font-size: 12pt;">Hi Levente,</span><br>
</p>
<div dir="ltr">
<div id="x_divtagdefaultwrapper" dir="ltr" style="font-size: 12pt; color: rgb(0, 0, 0); font-family: Calibri, Helvetica, sans-serif, EmojiFont, "Apple Color Emoji", "Segoe UI Emoji", NotoColorEmoji, "Segoe UI Symbol", "Android Emoji", EmojiSymbols;">
<p><br>
</p>
<p>many thanks for your review!</p>
<p><br>
</p>
<p></p>
<div>> Reading the source of a method - especially if you read the source of <span style="font-size:12pt">the same method multiple times, which is what you're trying to fix - </span><span style="font-size:12pt">should not take "several hundred milliseconds",
no matter how large </span><span style="font-size:12pt">your image is.</span></div>
<div><span style="font-size:12pt"><span style="font-family:Calibri,Helvetica,sans-serif,EmojiFont,"Apple Color Emoji","Segoe UI Emoji",NotoColorEmoji,"Segoe UI Symbol","Android Emoji",EmojiSymbols; font-size:16px"><br>
</span></span></div>
<div><span style="font-size:12pt"><span style="font-family:Calibri,Helvetica,sans-serif,EmojiFont,"Apple Color Emoji","Segoe UI Emoji",NotoColorEmoji,"Segoe UI Symbol","Android Emoji",EmojiSymbols; font-size:16px">That's indeed interesting.</span><br>
</span></div>
<div><span style="font-size:12pt"><br>
</span></div>
<div><span style="font-size:12pt">> Besides the obvious things, the runtime depends on a few </span><span style="font-size:12pt">things like:</span></div>
<div>> - the amount of free RAM there is,</div>
<div><span style="font-size:12pt">> </span><span style="font-size:12pt">- how well your OS caches files,</span><br>
</div>
<div></div>
<div><span style="font-size:12pt">> </span><span style="font-size:12pt">- whether the method's source has been cached or not,</span><br>
</div>
<div>
<div style="font-family:Calibri,Helvetica,sans-serif,EmojiFont,"Apple Color Emoji","Segoe UI Emoji",NotoColorEmoji,"Segoe UI Symbol","Android Emoji",EmojiSymbols; font-size:16px">
</div>
</div>
<div>> - the speed of the drive the source file is stored on,</div>
<div>> - whether you have too many open files and are close to the maxExternalSemaphores limit (which should have been bumped to 8k long ago),</div>
<div>but, in general, it should be in the submillisecond range.</div>
<div><br>
</div>
<div>
<div style="font-family:Calibri,Helvetica,sans-serif,EmojiFont,"Apple Color Emoji","Segoe UI Emoji",NotoColorEmoji,"Segoe UI Symbol","Android Emoji",EmojiSymbols; font-size:16px">
<div>80% - 90% of 16 RAM.</div>
Windows 10, SSD ... Setup should be rather state of the start.</div>
<div style="font-family:Calibri,Helvetica,sans-serif,EmojiFont,"Apple Color Emoji","Segoe UI Emoji",NotoColorEmoji,"Segoe UI Symbol","Android Emoji",EmojiSymbols; font-size:16px">
No additional antivirus software. Disabling OneDrive, which synces image & changes file in the background, accelerated the issue by around 30%, but it's still *much* slower than on your system.</div>
<div style="font-family:Calibri,Helvetica,sans-serif,EmojiFont,"Apple Color Emoji","Segoe UI Emoji",NotoColorEmoji,"Segoe UI Symbol","Android Emoji",EmojiSymbols; font-size:16px">
I tested it on a fresh Trunk image, so there shouldn't be a lot of files opened.</div>
</div>
<p></p>
<p><br>
</p>
<p></p>
<div>> but, in general, it should be in the submillisecond range.</div>
<div>> Here's an example reading the source of a rather large method (11kB):</div>
<div><br>
</div>
<div></div>
<span>[ (MethodFinder >> #initialize) getSource ] bench.</span>
<p></p>
<p><span>'11.4 per second. 87.7 milliseconds per run. 0.01214 % GC time.'</span></p>
<div></div>
<div><br>
</div>
Is this a typical workload?
<p></p>
<p><br>
</p>
<p><img naturalheight="377" naturalwidth="1400" size="127183" id="x_img151504" tabindex="0" style="max-width: 99.9%; user-select: none;" src="cid:ec96ec35-ab09-440a-b023-3210b7c34e7b"><br>
</p>
<p><br>
</p>
<p><span style="font-family:Calibri,Arial,Helvetica,sans-serif; font-size:12pt">> </span><span style="font-family:Calibri,Arial,Helvetica,sans-serif; font-size:12pt">> | styler |</span><br>
</p>
<div id="x_Signature">
<div id="x_divtagdefaultwrapper" dir="ltr" style="font-size:12pt; color:rgb(0,0,0); font-family:Calibri,Helvetica,sans-serif,EmojiFont,"Apple Color Emoji","Segoe UI Emoji",NotoColorEmoji,"Segoe UI Symbol","Android Emoji",EmojiSymbols">
<div name="x_divtagdefaultwrapper" style="font-family:Calibri,Arial,Helvetica,sans-serif; font-size:; margin:0">
<div>
<div class="x__rp_T4" id="x_Item.MessagePartBody">
<div>> > styler := SHTextStylerST80 new.</div>
<div>> > styler context: (thisContext stack at: 4). "to get a not-so-recently modified method"</div>
<div>> > [styler styledTextFor: 'foo' asText] bench</div>
<div>
<div>> Here's the result of the benchmark executed on my machine without your <span style="font-size:12pt">changes:</span></div>
<div>> <span style="font-size:12pt"> '4,700 per second. 213 microseconds per run. 1.04 % GC time.'</span></div>
<br>
</div>
<div>How fast is the example if you do load my example?</div>
<div>
<p>In general, do you think my proposal makes any sense for "normal" systems?</p>
<div id="x_Signature">
<div id="x_divtagdefaultwrapper" dir="ltr" style="font-size:12pt; font-family:Calibri,Helvetica,sans-serif,EmojiFont,"Apple Color Emoji","Segoe UI Emoji",NotoColorEmoji,"Segoe UI Symbol","Android Emoji",EmojiSymbols">
<div name="x_divtagdefaultwrapper" style="font-family:Calibri,Arial,Helvetica,sans-serif; margin:0px">
<div class="x__rp_T4" id="x_Item.MessagePartBody">
<div class="x__rp_U4 x_ms-font-weight-regular x_ms-font-color-neutralDark x_rpHighlightAllClass x_rpHighlightBodyClass" id="x_Item.MessageUniqueBody" style="font-family:wf_segoe-ui_normal,"Segoe UI","Segoe WP",Tahoma,Arial,sans-serif,serif,EmojiFont">
<div dir="ltr">
<div id="x_divtagdefaultwrapper"><font face="Calibri,Helvetica,sans-serif,EmojiFont,Apple Color Emoji,Segoe UI Emoji,NotoColorEmoji,Segoe UI Symbol,Android Emoji,EmojiSymbols">
<div id="x_Signature">
<div style="margin:0px"><font style="font-family:Calibri,Arial,Helvetica,sans-serif,serif,EmojiFont"></font></div>
</div>
</font></div>
</div>
</div>
</div>
<div class="x__rp_T4" id="x_Item.MessagePartBody"><br>
</div>
<div class="x__rp_T4" id="x_Item.MessagePartBody">> <span style="font-size:12pt">I know I sound like a broken record, but if you care about performance, </span><span style="font-size:12pt">don't use symbols in place of blocks, especially as the arguments of </span><span style="font-size:12pt">methods
like #ifNotNil:, which would optimize the blocks away.</span>
<div><br>
</div>
<div>Optimization is a good point (although I have the impression that this might be rather premature optimization). Also, we could optimize symbols in the same way as blocks, couldn't we? But I will try to keep it in mind, also in terms of readability :)</div>
<div><br>
</div>
<div>> <span style="font-size:12pt">While #take: works, I would keep using #first: here. We know that the </span><span style="font-size:12pt">receiver is String (or at least it should be) and we need to get the first </span><span style="font-size:12pt">N characters.
We can also assume that contextSourcePcIndex is within the </span><span style="font-size:12pt">range of the source. If it's not, then it's very likely a bug that would </span><span style="font-size:12pt">be hidden by #take:.</span>
<div><br>
</div>
<div>Actually, I got a "subscript out of bounds" a few times, so there might be indeed a bug somewhere. Do we really want Shout to be the bug-checking instance here? We also could raise a warning if the source is too short, without irritating all kind of users
with a non-working toolset (because this would likely crash the debugger as well). I don't know what's the typical approach in Squeak to balance robustness vs error detection ...</div>
<div><br>
</div>
<div>Best,</div>
<div>Christoph</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
<div><font size="2" color="#808080"></font></div>
</div>
</div>
</div>
</div>
<hr tabindex="-1" style="display:inline-block; width:98%">
<div id="x_divRplyFwdMsg" dir="ltr"><font face="Calibri, sans-serif" color="#000000" style="font-size:11pt"><b>Von:</b> Squeak-dev <squeak-dev-bounces@lists.squeakfoundation.org> im Auftrag von Levente Uzonyi <leves@caesar.elte.hu><br>
<b>Gesendet:</b> Sonntag, 12. Januar 2020 19:04:35<br>
<b>An:</b> squeak-dev@lists.squeakfoundation.org<br>
<b>Betreff:</b> Re: [squeak-dev] The Inbox: ShoutCore-ct.78.mcz</font>
<div> </div>
</div>
</div>
<font size="2"><span style="font-size:10pt">
<div class="PlainText">Hi Christoph,<br>
<br>
On Sun, 12 Jan 2020, commits@source.squeak.org wrote:<br>
<br>
> Christoph Thiede uploaded a new version of ShoutCore to project The Inbox:<br>
> <a href="http://source.squeak.org/inbox/ShoutCore-ct.78.mcz" id="LPlnk47316" previewremoved="true">
http://source.squeak.org/inbox/ShoutCore-ct.78.mcz</a><br>
><br>
> ==================== Summary ====================<br>
><br>
> Name: ShoutCore-ct.78<br>
> Author: ct<br>
> Time: 12 January 2020, 3:00:21.215555 am<br>
> UUID: c04a2f92-7a12-1743-a4d7-5d555c8ee634<br>
> Ancestors: ShoutCore-mt.77<br>
><br>
> Optimize context-dependent styling of multiple texts in the same context.<br>
><br>
> Reading the context's method from the sources file can be *really* slow (up to several hundred milliseconds, depending on the size of your image). Thus we don't need to request the sources file again each time we handle a subsequent styling request for the
same context.<br>
<br>
Reading the source of a method - especially if you read the source of <br>
the same method multiple times, which is what you're trying to fix - <br>
should not take "several hundred milliseconds", no matter how large <br>
your image is. Besides the obvious things, the runtime depends on a few <br>
things like:<br>
- the amount of free RAM there is,<br>
- how well your OS caches files,<br>
- whether the method's source has been cached or not,<br>
- the speed of the drive the source file is stored on,<br>
- whether you have too many open files and are close to the maxExternalSemaphores limit (which should have been bumped to 8k long ago),<br>
but, in general, it should be in the submillisecond range.<br>
Here's an example reading the source of a rather large method (11kB):<br>
<br>
[ (MethodFinder >> #initialize) getSource ] bench.<br>
'11,800 per second. 84.5 microseconds per run. 5.95762 % GC time.'<br>
<br>
><br>
> Measurements from a fresh Trunk image:<br>
><br>
> | styler |<br>
> styler := SHTextStylerST80 new.<br>
> styler context: (thisContext stack at: 4). "to get a not-so-recently modified method"<br>
> [styler styledTextFor: 'foo' asText] bench<br>
><br>
> Before: 36 milliseconds/run<br>
> After: 161 microseconds/run<br>
<br>
Here's the result of the benchmark executed on my machine without your <br>
changes:<br>
<br>
'4,700 per second. 213 microseconds per run. 1.04 % GC time.'<br>
<br>
So, perhaps there's some other thing causing the slowdown, e.g. one of the <br>
reasons I mentioned above or some anti-virus software.<br>
<br>
><br>
> Cache is fast:<br>
><br>
> [| styler |<br>
> styler := SHTextStylerST80 new.<br>
> styler context: (thisContext stack at: 4). "to get a not-so-recently modified method"<br>
> styler styledTextFor: 'foo' asText] bench<br>
><br>
> Before: 276 milliseconds/run<br>
> After: 276 milliseconds/run "sic!"<br>
><br>
> =============== Diff against ShoutCore-mt.77 ===============<br>
><br>
> Item was changed:<br>
> Object subclass: #SHParserST80<br>
> + instanceVariableNames: 'source parseAMethod classOrMetaClass workspace environment context arguments sourcePosition currentToken currentTokenFirst temporaries instanceVariables errorBlock currentTokenSourcePosition bracketDepth ranges allowUnderscoreAssignments
allowUnderscoreSelectors allowBlockArgumentAssignment currentTokenType contextMethodCache'<br>
> - instanceVariableNames: 'classOrMetaClass source workspace arguments sourcePosition currentToken currentTokenFirst temporaries instanceVariables errorBlock currentTokenSourcePosition bracketDepth ranges environment allowUnderscoreAssignments allowUnderscoreSelectors
allowBlockArgumentAssignment parseAMethod currentTokenType context'<br>
<br>
The new instance variable should be documented in the class comment.<br>
<br>
> classVariableNames: ''<br>
> poolDictionaries: ''<br>
> category: 'ShoutCore-Parsing'!<br>
><br>
> !SHParserST80 commentStamp: 'ul 7/30/2019 00:31' prior: 0!<br>
> I am a Smalltalk method / expression parser.<br>
><br>
> Rather than creating an Abstract Syntax Tree, I create a sequence of SHRanges (in my 'ranges' instance variable), which represent the tokens within the String I am parsing.<br>
><br>
> I am used by a SHTextStylerST80 to parse method source strings.<br>
> I am able to parse incomplete / incorrect methods, and so can be used to parse methods that are being edited.<br>
><br>
> Instance Variables<br>
> allowBlockArgumentAssignment: <Boolean><br>
> allowUnderscoreAssignments: <Boolean><br>
> allowUnderscoreSelectors: <Boolean><br>
> arguments: <OrderedCollection<OrderedCollection<String>|nil><br>
> bracketDepth: <Integer><br>
> classOrMetaClass: <Class|nil><br>
> currentToken: <String|nil><br>
> currentTokenFirst: <Character><br>
> currentTokenSourcePosition: <Integer|nil><br>
> currentTokenType: <Symbol|nil><br>
> environment: <Environment><br>
> errorBlock: <Block><br>
> instanceVariables: <Array><br>
> parseAMethod: <Boolean><br>
> ranges: <OrderedCollection<SHRange>><br>
> source: <String><br>
> sourcePosition: <Integer><br>
> temporaries: <OrderedCollection<OrderedCollection<String>|nil><br>
> workspace: <Workspace|nil><br>
> context: <Context|nil><br>
><br>
> allowBlockArgumentAssignment<br>
> The value cached at the beginning of parsing of Scanner allowBlockArgumentAssignment.<br>
><br>
> allowUnderscoreAssignments<br>
> The value cached at the beginning of parsing of Scanner allowUnderscoreAsAssignment.<br>
><br>
> allowUnderscoreSelectors<br>
> The value cached at the beginning of parsing of Scanner prefAllowUnderscoreSelectors.<br>
><br>
> arguments<br>
> This OrderedCollection has an element for each scope encapsulating the current scope.<br>
> The current scope's arguments are stored in the last element. The first element holds the outermost scope's arguments.<br>
> Each element is nil when the corresponding scope doesn't have any arguments, and the element is an OrderedCollection with the names of the arguments declared at the given scope when there's at least one.<br>
> The size of this variable is the same as the size of temporaries.<br>
><br>
> bracketDepth<br>
> Stores the number of unclosed brackets "(" and parentheses "[" before the current sourcePosition.<br>
><br>
> classOrMetaClass<br>
> The Class or MetaClass instance, class and pool variables should be looked up during parsing or nil when not parsing code in the context of a class (e.g. when parsing code written in a Workspace). Having this set doesn't mean a method is being parsed.<br>
><br>
> currentToken<br>
> The token being analyzed for which the next range should be created for.<br>
><br>
> currentTokenFirst<br>
> The first character of currentToken cached for quick access or a space character when there are no more tokens to parse.<br>
> Being always a Character helps avoiding extra checks.<br>
><br>
> currentTokenSourcePosition<br>
> The position of source the current token starts at or nil when there are no more tokens to process.<br>
><br>
> currentTokenType<br>
> The type of the current token calculated lazily by #currentTokenType. When it has been calculated, Its value is one of #keyword, #assignment, #ansiAssignment, #binary, #name, #other and occasionally #invalid.<br>
><br>
> environment<br>
> The Environment globals and classes should be looked up at during parsing when classOrMetaClass is nil. Its value is Smalltalk globals by default.<br>
><br>
> errorBlock<br>
> A block used to quickly stop parsing in case of an unrecoverable parse error.<br>
><br>
> instanceVariables<br>
> An Array with the instance variable names of classOrMetaClass or an empty Array when classOrMetaClass is nil.<br>
><br>
> parseAMethod<br>
> A way to tell the parser to parse source as a code snippet instead of a method. Mainly used by inspectors.<br>
><br>
> ranges<br>
> The SHRanges parsed by the parser.<br>
><br>
> source<br>
> The source code as a String to be parsed.<br>
><br>
> sourcePosition<br>
> souce is treated as a stream by the parser. This variable stores the stream position.<br>
><br>
> temporaries<br>
> This OrderedCollection has an element for each scope encapsulating the current scope.<br>
> The current scope's temporaries are stored in the last element. The first element holds the outermost scope's temporaries.<br>
> Each element is nil when the corresponding scope doesn't have any temporary variables, and the element is an OrderedCollection with the names of the temporaries declared at the given scope when there's at least one.<br>
> The size of this variable is the same as the size of arguments.<br>
><br>
> workspace<br>
> The Workspace in whose context variables should be looked up during parsing or nil when not parsing code in a workspace.<br>
><br>
> context<br>
> The Context in which variables should be looked up during parsing or nil when not parsing within a context.<br>
><br>
> Example (explore it):<br>
><br>
> ranges := SHParserST80 new<br>
> classOrMetaClass: Object;<br>
> source: 'testMethod ^self';<br>
> parse;<br>
> ranges<br>
><br>
> Benchmark (print it):<br>
><br>
> SHParserST80 benchmark!<br>
><br>
> Item was changed:<br>
> ----- Method: SHParserST80>>initializeVariablesFromContext (in category 'parse support') -----<br>
> initializeVariablesFromContext<br>
> <br>
> + | contextMethod contextSource contextSourcePcIndex contextSourceParser |<br>
> + contextMethod := context method.<br>
> + (contextMethodCache ifNotNil: #key) = contextMethod "for optimization"<br>
<br>
I know I sound like a broken record, but if you care about performance, <br>
don't use symbols in place of blocks, especially as the arguments of <br>
methods like #ifNotNil:, which would optimize the blocks away.<br>
Also, using a symbol with #ifNotNil: may not legible to the average <br>
Smalltalker. I'm sure some would expect [ x ifNotNil: #y ] to yield #y <br>
when x is not nil.<br>
<br>
> + ifFalse: [contextMethodCache := contextMethod -> contextMethod getSource].<br>
> + contextSource := contextMethodCache value.<br>
> + <br>
> - | contextSourcePcIndex contextSourceParser |<br>
> contextSourcePcIndex := (context debuggerMap<br>
> rangeForPC: (context isDead ifTrue: [context endPC] ifFalse: [context pc])<br>
> in: context method<br>
> contextIsActiveContext: true "... to really use the context's pc.")<br>
> start.<br>
> contextSourceParser := self class new<br>
> classOrMetaClass: context method methodClass;<br>
> + environment: context receiver environment;<br>
> + source: (contextSource take: contextSourcePcIndex);<br>
<br>
We should stop passing Text obejcts as source to SHParserST80. It only <br>
works on Strings. Eliot added a guard recently because of the <br>
introduction of texts, and I wanted to fixed that, but it seems I never <br>
got to do that.<br>
CompiledMethod >> #getSource returns a Text, so the best place to convert <br>
to String is right after sending #getSource. That would also save us a few <br>
CPU cycles.<br>
<br>
While #take: works, I would keep using #first: here. We know that the <br>
receiver is String (or at least it should be) and we need to get the first <br>
N characters. We can also assume that contextSourcePcIndex is within the <br>
range of the source. If it's not, then it's very likely a bug that would <br>
be hidden by #take:.<br>
<br>
<br>
Levente<br>
<br>
> - environment: self environment;<br>
> - source: (context method getSource first: contextSourcePcIndex);<br>
> yourself.<br>
> contextSourceParser parse.<br>
> arguments := contextSourceParser activeArguments.<br>
> temporaries := contextSourceParser activeTemporaries.!<br>
<br>
</div>
</span></font></div>
</body>
</html>