[squeak-dev] The Inbox: ShoutCore-ct.78.mcz

Levente Uzonyi leves at caesar.elte.hu
Mon Jan 13 22:19:06 UTC 2020


Hi Christoph,

On Mon, 13 Jan 2020, Thiede, Christoph wrote:

> 
> Hi Levente,
> 
> 
> many thanks for your review!
> 
> 
> > Reading the source of a method - especially if you read the source of the same method multiple times, which is what you're trying to fix
> - should not take "several hundred milliseconds", no matter how large your image is.
> 
> That's indeed interesting.
> 
> > Besides the obvious things, the runtime depends on a few things like:
> > - the amount of free RAM there is,
> > - how well your OS caches files,
> > - whether the method's source has been cached or not,
> > - the speed of the drive the source file is stored on,
> > - whether you have too many open files and are close to the maxExternalSemaphores limit (which should have been bumped to 8k long ago),
> but, in general, it should be in the submillisecond range.
> 
> 80% - 90% of 16 RAM.
> Windows 10, SSD ... Setup should be rather state of the start.
> 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.
> I tested it on a fresh Trunk image, so there shouldn't be a lot of files opened.
> 
> 
> > but, in general, it should be in the submillisecond range.
> > Here's an example reading the source of a rather large method (11kB):
> 
> [ (MethodFinder >> #initialize) getSource ] bench.
> 
> '11.4 per second. 87.7 milliseconds per run. 0.01214 % GC time.'
> 
> 
> Is this a typical workload?
> 
> 
> [IMAGE]
>

Clearly not. It takes way too long on your machine to open a file. Last 
time I used Squeak on Windows was a while ago, but there was no issue like 
that.
It's worth investigating why that takes so long. Perhaps its a VM issue.

> 
> > >        | styler |
> 
> > >        styler := SHTextStylerST80 new.
> > >        styler context: (thisContext stack at: 4). "to get a not-so-recently modified method"
> > >        [styler styledTextFor: 'foo' asText] bench
> > Here's the result of the benchmark executed on my machine without your changes:
> >   '4,700 per second. 213 microseconds per run. 1.04 % GC time.'
> 
> How fast is the example if you do load my example?

It's about 1.24% faster, around 163 microseconds per run.

> 
> In general, do you think my proposal makes any sense for "normal" systems?

On "normal" systems, not really. Based on my measurements and assuming a 
100x slowdown, I expect it to save 4ms per run on SqueakJS (from 20ms to 
16ms).
It sounds like premature optimization, so I'd at least add a comment about 
why it was added. And I would try to find the cause of why it takes so 
long on your machine to open a file.

Here's a small benchmark I just came up with:

[ | filename |
filename := UUID new asString36.
FileStream newFileNamed: filename do: [ :file | ].
[ FileStream fileNamed: filename do: [ :file | ] ] bench ]
 	ensure: [ FileDirectory default deleteFileNamed: filename ].

'119,000 per second. 8.43 microseconds per run. 4.27914 % GC time.'

> 
> 
> > I know I sound like a broken record, but if you care about performance, don't use symbols in place of blocks, especially as the arguments
> of methods like #ifNotNil:, which would optimize the blocks away.
> Optimization is a good point (although I have the impression that this might be rather premature optimization). Also, we could optimize symbols in

Premature optimization involves two things:
- added complexity
- optimizing for a possible, but currently not existant or unlikely use 
case
Neither seem to apply here.

You could argue that it's an unnecessary optimization, as it won't give 
measurable difference. I simply consider it good practice to write things 
an optimal way if it is straightforward, like in that case.
All in all, it costs nothing to write a block there.

> the same way as blocks, couldn't we? But I will try to keep it in mind, also in terms of readability :)

We probably could but why would we? :)

> 
> > While #take: works, I would keep using #first: here. We know that the receiver is String (or at least it should be) and we need to get the
> first N characters. We can also assume that contextSourcePcIndex is within the range of the source. If it's not, then it's very likely a bug that
> would be hidden by #take:.
> 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 ...

#take: clearly just makes things work at the cost of hiding the bug.
We should really know why the source is shorter than the index returned 
by DebuggerMethodMap. I've never seen the bug myself, but it's something 
worth debugging.


Levente

> 
> Best,
> Christoph


More information about the Squeak-dev mailing list