Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fewer MOBILEAPP Directive #10181

Merged
merged 4 commits into from
Oct 10, 2024
Merged

Fewer MOBILEAPP Directive #10181

merged 4 commits into from
Oct 10, 2024

Conversation

Ashod
Copy link
Contributor

@Ashod Ashod commented Oct 7, 2024

  • wsd: restore performance regression due to isMobileApp
  • wsd: remove ifdef MOBILEAPP
  • wsd: remove ifdef MOBILEAPP
  • wsd: header cleanup
  • wsd: remove ifdef MOBILEAPP

@Ashod Ashod changed the title private/ash/mobile Fewer MOBILEAPP Directive Oct 7, 2024
@Ashod Ashod requested a review from caolanm October 7, 2024 01:17
wsd/DocumentBroker.cpp Fixed Show fixed Hide fixed
@Ashod Ashod force-pushed the private/ash/mobile branch 2 times, most recently from 016b76a to 10467ee Compare October 7, 2024 01:41
Copy link
Contributor

@caolanm caolanm left a comment

Choose a reason for hiding this comment

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

principle looks good, but current compile error on mobile platforms of:

wsd/Storage.cpp:86:13: error: use of undeclared identifier 'HostUtil'
HostUtil::parseWopiHost(app.config());

Previously, mobile-app code was gated behind compiler
directives, which compiled the code in and out as
necessary. This was replaced by runtime conditionals.

These conditionals are evaluated at runtime, which
of course comes at a cost (presumably the predictor
will reduce this cost in most cases). In addition,
there is increased code, which wastes the hardware
caches unnecessarily.

Luckily, we can have the benfits of both worlds
by leveraging constexpr. Now, these conditionals
will be evaluated at compile-time, much like
the compiler directives, restoring whatever
lost performance while keeping the code clean.

Change-Id: Ied55ec2a4ece6d964da4e0e2a40fbe73eec99a46
Signed-off-by: Ashod Nakashian <[email protected]>
Change-Id: Id2e3a8cee3935090eb8e9f310bb72ce659f1966d
Signed-off-by: Ashod Nakashian <[email protected]>
@Ashod Ashod force-pushed the private/ash/mobile branch 4 times, most recently from 7bcb4ca to b6ed599 Compare October 8, 2024 01:28
Change-Id: I2ea8895abdeb5c2de0812e9494239bc2dbbfb99a
Signed-off-by: Ashod Nakashian <[email protected]>
Change-Id: I7c7a425fde746bf97b833c552bdd31c8bff221ca
Signed-off-by: Ashod Nakashian <[email protected]>
@Ashod
Copy link
Contributor Author

Ashod commented Oct 8, 2024

principle looks good, but current compile error on mobile platforms of:

wsd/Storage.cpp:86:13: error: use of undeclared identifier 'HostUtil' HostUtil::parseWopiHost(app.config());

Yes, some of these can't be converted to constexpr conditionals. I'm learning the hard-way and reverting selectively.
Others are impossible to convert altogether (conditionals outside of functions, around includes, etc.).
Still, any reduction of compiler directives seems a win.

Copy link
Contributor

@caolanm caolanm left a comment

Choose a reason for hiding this comment

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

Looks good to me

@Ashod Ashod merged commit e4b5131 into master Oct 10, 2024
14 checks passed
@Ashod Ashod deleted the private/ash/mobile branch October 10, 2024 11:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants