Skip to content

Conversation

@oleksandr-nc
Copy link
Contributor

Results for 5k requests:

OCS call, AppAPI enabled(before PR):

Maximum request time: 0.01429 seconds
Minimum request time: 0.01301 seconds
Average request time: 0.01395 seconds

OCS call, AppAPI enabled(after PR):

Maximum request time: 0.01383 seconds
Minimum request time: 0.01281 seconds
Average request time: 0.01347 seconds (3.5% performance boost compared to old code)

For frontend calls the speed is almost the same, slight boost with less then 1%, when two ExApps define their icons.

Why this code is writtent without usual initilizing in the __construct()?

$menuEntries = Server::get(TopMenuService::class)->getExAppMenuEntries();
...
/** @var IGroupManager $groupManager */
$groupManager = Server::get(IGroupManager::class);
/** @var INavigationManager $navigationManager */
$navigationManager = Server::get(INavigationManager::class);
/** @var IURLGenerator $urlGenerator */
$urlGenerator = Server::get(IURLGenerator::class);
/** @var IFactory $l10nFactory */
$l10nFactory = Server::get(IFactory::class);

This is much faster, as this does not initialize all this classes for each request, but only initialize them for those requests where we should do the actual work.
First version of this PR was written in a nice way(with private readonly ... in constructor), but after measurments the code was completly rewritten.

@oleksandr-nc oleksandr-nc force-pushed the fix/perf/ExAppTopMenu-event branch from 998735f to 46b7064 Compare April 4, 2025 11:18
@oleksandr-nc
Copy link
Contributor Author

question - will we backport this to the NC31 or not?

Copy link
Collaborator

@kyteinsky kyteinsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀

question - will we backport this to the NC31 or not?

okay to do imo

@nickvergessen
Copy link
Member

Why this code is writtent without usual initilizing in the __construct()?

I don't get that. You only give the class name to the register listener. The listener is not initialized until the event is actually triggered.
If that is no longer the case it's a serious bug.

@oleksandr-nc
Copy link
Contributor Author

I don't get that. You only give the class name to the register listener. The listener is not initialized until the event is actually triggered.
If that is no longer the case it's a serious bug.

Ok, I will recheck that.

…event is triggered

Signed-off-by: Oleksander Piskun <oleksandr2088@icloud.com>
@oleksandr-nc oleksandr-nc force-pushed the fix/perf/ExAppTopMenu-event branch from 46b7064 to d505473 Compare April 4, 2025 13:36
@oleksandr-nc
Copy link
Contributor Author

I forced-pushed a slightly modified version.

You are right, the class is not initialized until there is an event, so we can put TopMenuService in the constructor, because it will always be used first during an event and it makes no difference where it is located.

After taking measurements when there are ExApps but none of them define a menu item, the code without IGroupManager, INavigationManager, IURLGenerator, IFactory in the constructor still works faster (or it is a measurement error as difference value is not so big).

I would like to leave them where they are, if this does not cause any problems that I do not know about - but if it is needed, I can push a version where they are initialized in the constructor.

@oleksandr-nc oleksandr-nc merged commit bf4085d into main Apr 6, 2025
36 checks passed
@oleksandr-nc oleksandr-nc deleted the fix/perf/ExAppTopMenu-event branch April 6, 2025 09:45
@oleksandr-nc
Copy link
Contributor Author

/backport to stable31

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants