In a recent post on the Pharo mailing list, Jannik noticed that when opening the System Browser on a single package settings, it is really slow. What could the problem be?
Let’s see. I actually do not know anything about the internal implementation of the browser, but we can start from the line that came with the original report:
SettingBrowser new setViewedPackageNames: 'Settings-Network'; open.
To investigate the problem, we use the GTInspector support for message tally.
Inspecting:
MessageTally new spyEvery: 1 on: [ SettingBrowser new setViewedPackageNames: 'Settings-Network'; open ]; yourself
reveals a 10s delay until opening the window. Indeed, something looks strange.
Investigating the execution further shows that the problem is related to applying filtering in updateAccordingTo:
.
Going a step back, we notice that the filter is being applied only when viewedPackages
is not empty. We get confident that we am on the right path.
At a deeper inspection, we see that PackageOrganizer
gets involved. That rings a bell: Pharo 3.0 comes with the RPackage
support exactly to fix the problems of PackageOrganizer
.
Thus, one thing to do is to use RPackage
. For this we need two things:
Armed with this knowledge, we can modify:
SettingNode>>receiverPackage ^ self settingReceiver ifNotNil: [self settingReceiver class package]
and:
SettingBrowser>>setViewedPackageNames: aText | allViewed | allViewed := Set new. aText asString substrings do: [:sub | (RPackageOrganizer default packageNamed: sub ifAbsent: []) ifNotNil: [:pkg | allViewed add: pkg]]. self changePackageSet: allViewed. self changed: #getViewedPackageNames
Did it solve the problem? Re-running the snippet after the modifications, gets us a 1.8s result:
Great! It’s probably not the best we could do, but it becomes reasonable. Yet, is the job ready?
Not quite. We only modified things that were related to the exercised use case. What if there are other parts in the implementation that depend on the old PackageOrganizer
and related classes? Of course, we could run all the tests, but unfortunately there are no tests for this browser. Or we could just click around, but we would still not know if we clicked enough.
Or we could search for what we know already: all references to PackageOrganizer
and PackageInfo
should be replaced:
(RPackageOrganizer default packageNamed: 'System-Settings') methodReferences select: [:each | | ast | ast := each method parseTree. (ast references: PackageOrganizer name) or: [ ast references: PackageInfo name ] ]
This gets us one method:
SettingNode>>package
^ self methodClass ifNotNil: [:mc | PackageOrganizer default mostSpecificPackageOfClass: mc ifNone: []]
Looking at the senders, reveals that the method is actually not used (the most relevant user is the one we just wrote):
We remove the method. And we are almost there. There is still one thing left we could do. We could search for all messages that sent from the Settings Browser and that are implemented in PackageOrganizer
but are not implemented in RPackageOrganizer
:
(RPackageOrganizer default packageNamed: 'System-Settings') methodReferences select: [:each | each method parseTree allChildren anySatisfy: [ :node | node isMessage and: [ (PackageOrganizer selectors includes: node selector) and: [ (RPackageOrganizer selectors includes: node selector) not ] ] ] ] ]
This gets zero hits. But, repeating the same query for PackageInfo
and RPackage
gets us 24 candidate methods:
However, going a step further, we see that the problematic messages are actually not RPackage specific at all:
Now we are done.
This took about half an hour of work, and at the end of it we improved the performance of a piece of code we never saw before. We did this by systematically digging through the problem and by using appropriate analyses for performance and static checks. Along the way, code reading was used sparingly and driven by a hypothesis and custom tools.
The ability to not read code all the time is invaluable for getting productive results.