Merged
Conversation
strseb
requested review from
artfwo and
encrypt94
and removed request for
encrypt94
strseb
force-pushed
the
basti/feature_list_refactor
branch
6 times, most recently
from
727f9a9 to
6ab3b4f
Compare
The "feature.h" as is is a bit heavy: -> We do have a new Allocation of Feature for every feature -> We do track the pointers in both a hashmap and qlist -> Each feature QString is created from a UTF-8 char, so each QString needs an allocation too! Add to that that the evaluation algorightm of feature -> bool (is this on?) - needs to check multiple std::functions a String Based List lookup for possible dependencies. A roundtrip to a settingslist to check if we ever feature-flip from guardian. No optimization is possible, even if we mark a Feature with "FeatureCallback_false" - given we construct the list at runtime, fetch a Feature* via a QString and then check a std::func* - these are too many layers for the compiler to determine "this is always false". The reality is, the majority of our feature-flags are Fixed and Non Changeable by the time the Compiler has evaluated the defines. So we are paying a lot to have flexibility we actually never use and we are paying that on every start of the app. My proposal is to swap this into a "Pay What you need" model with 3 Tiers of FeatureFlags: -> Static Feature: They are determined at compiletime. i.e "is this windows?" They need no allocation, just live in the data section. They are constexpr the compiler can do code-elimination on dead codepaths. -> Runtime Feature: A wrapper around func -> Bool; They are run on every check, i.e "deactivate windows-split-tunnel when mullvad is detected". -> Dynamic Feature: A simplified version of our current Feature*, it has a func->bool for the base value. Overrides via the dev-menu are possibile. Also they support overrides from guardian. As none of these features are QObjects, for FeatureModel i add a "ViewModel" Class FeatureProxy(feature::any) that can take a ref for any feature and expose a Feature to QML. The API for QML is unchanged, only in c++ land we have to change callers.
strseb
force-pushed
the
basti/feature_list_refactor
branch
2 times, most recently
from
eb7e4aa to
14bbe0b
Compare
strseb
force-pushed
the
basti/feature_list_refactor
branch
from
14bbe0b to
4d78df7
Compare
strseb
changed the title
mcleinman
reviewed
Apr 24, 2026
Collaborator
mcleinman
left a comment
mcleinman
left a comment
There was a problem hiding this comment.
One tiny tiny nit. Thanks for this.
| #endif | ||
|
|
||
| #ifdef MZ_LINUX | ||
| inline constexpr bool linux_ = true; |
Collaborator
There was a problem hiding this comment.
Such a tiny nit, sorry - but this is linux_?
Collaborator
Author
There was a problem hiding this comment.
just linux is a reserved keyword in gcc 😰 - i though about gnu_linux, but not sure if editors would
auto suggest that if you type plattform::linux.
mcleinman
approved these changes
Apr 24, 2026
Collaborator
mcleinman
left a comment
mcleinman
left a comment
There was a problem hiding this comment.
That was supposed to approval w/ that non-blocking nit. Sorry.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
feature.h costs more than it should. Every feature allocates a Featurethat lives in both a hashmap and a QList. Every string field starts with UTF-8 char and gets parsed into to a QString — one temp allocation per field, per feature, at startup.
FeatureList::MaybeInit ends up doing roughly as many temp allocations as the entire MozillaVPN constructor.
Evaluation isn't free either. Feature::isEnabled walks through multiple std::functions, a string-based dependency lookup, and a SettingsHolder round-trip in case Guardian ever flipped the feature. Even a feature hard-wired to
FeatureCallback_false goes through the full pipeline — the compiler can't
prove "always false" because the list is built at runtime.
But most of our flags are hard-coded against compile defines.
We're paying for flexibility we never use, on every start and every check.
My proposal is to swap this into a "Pay What you need" model with 3 Tiers of FeatureFlags:
-> ConstantFeature : Value is known at compile time (Platform::windows, Qt version, build defines). constexpr — zero allocation, fully foldable by the compiler.
-> RuntimeFeature - Value needs a runtime check but never flips (e.g. "disable Windows split-tunnel when Mullvad is detected", Android SDK version). One function-pointer call.
-> Dynamic Feature: OverridableFeature, Dev-menu and Guardian (/api/v1/features) can override. Function-pointer call + settings lookup, only when you opt in.
None of the feature structs are QObjects. For QML, a small FeatureProxy
view-model wraps any AnyFeature reference and exposes the same API QML was
already using — so MZFeatureList.get("foo").isSupported still works
unchanged.
I've split this into 2 Commits to make it nicer to review, part 1 only concerns changes in
/feature-part 2 is a bulk change for consumers of the featurelist.