pharo-project / pharo

Pharo is a dynamic reflective pure object-oriented language supporting live programming inspired by Smalltalk.
http://pharo.org
Other
1.21k stars 355 forks source link

Enhance Categorizer/SystemOrganizer by switching from three Arrays to an OrderedDictionary #12602

Closed pdebruic closed 1 year ago

pdebruic commented 1 year ago

Describe the request I have a package with ~580 classes in 78 categories. In Pharo 10, for a class in that package, opening a browser takes 1.5 seconds, changing from flat to hierarchical view takes 3.6 seconds, according to TimeProfiler.

Doing the same browsing operations in e.g. System-Support-Image package is instantaneous.

The TimeProfiler shows the majority of the time is spent in the Categorizer>>listAtCategoryNamed: method, which does an indexOf:ifAbsent: lookup.

From the looks of the Categorizer class it seems like there is a lot of accounting code for keeping track of where each category's class lists begin and end which could just be a simple array as the value in the dictionary. Key = category name, value = class list

To see for yourself run:

RPackageOrganizer default createPackageNamed: 'Foo'.
1 to:600 do:[:ind | |classNameSuffix categoryNameSuffix |
     classNameSuffix :=ind asString. 
    categoryNameSuffix :=(ind//8) asString.
    Object subclass: ('Foo' , classNameSuffix) asSymbol
    instanceVariableNames: ''
    classVariableNames: ''
    package: 'Foo-Data-',categoryNameSuffix  ].

Then try to browse one of those classes and also switch from flat to hierarchical view.

Expected behavior no degradation in speed of browsing or class display as packages increase in size...... I guess.

Expected development cost Less than half a day? I don't know how to swap in a different version of the SystemOrganizer or what relies on it if anything now.

Version information:

pdebruic commented 1 year ago

Oh and to use the TimeProfiler I edited

ClySpawnFullBrowserCommand>>#execute
TimeProfiler spyOn:[
    browser spawnFullBrowser]

and

ClySwitchToFullHierarchyCommand>>#execute
TimeProfiler spyOn:[
    browser switchToFullClassHierarchy]
pdebruic commented 1 year ago

also I think it slows adding inst vars or compiling classes but I'm unsure how to profile those actions

Ducasse commented 1 year ago

Thanks Paul. Indeed we should improve.

jecisc commented 1 year ago

Hi, I'll take a look at that in Pharo 12 because there are multiple cleanings I would like to do on that part of the system. 1) I'd like to remove everything related to protocols because this categorizer does not manage protocols anymore 2) Merge it with SystemOrganizer because this subclass is the only one used and it would make the kernel a little smaller 3) Use a dictionary as you suggested

jecisc commented 1 year ago

I have an implementation here: https://github.com/jecisc/pharo/tree/p12/categorizer/use-dictionary-in-categorizer

With that, it takes 300ms after using you script instead of 3.5sec. This is way better. When the Pharo 12 branch will work I'll do a PR

GitHub
GitHub - jecisc/pharo at p12/categorizer/use-dictionary-in-categorizer
Contribute to jecisc/pharo development by creating an account on GitHub.
pdebruic commented 1 year ago

Looks like I could copy it into my own pharo10 images. Is there anything that would prevent that?

jecisc commented 1 year ago

Yeah. I changed the variables of the system organizer and I guess that if you do that you'll crash your image because the categories will be lost :(

MarcusDenker commented 1 year ago

merged