Merged
Conversation
…rformance (#534) ## Description After migrating llms to cpp architecture we saw significant drop in performance (up to 10x slower). After debugging the main culprit turned out to be the number of threads spawned for xnnpack threadpool, which defaulted to the number of cores (the underlying reason is still unknown). ### Introduces a breaking change? - [x] Yes - migrating back to cpp architecture for llms - [ ] No ### Type of change - [x] Bug fix (change which fixes an issue) - [x] New feature (change which adds functionality) - [ ] Documentation update (improves or adds clarity to existing documentation) - [ ] Other (chores, tests, code style improvements etc.) ### Tested on - [x] iOS - [x] Android ### Testing instructions <!-- Provide step-by-step instructions on how to test your changes. Include setup details if necessary. --> ### Screenshots <!-- Add screenshots here, if applicable --> ### Related issues <!-- Link related issues here using #issue-number --> ### Checklist - [ ] I have performed a self-review of my code - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] I have updated the documentation accordingly - [ ] My changes generate no new warnings ### Additional notes <!-- Include any additional information, assumptions, or context that reviewers might need to understand this PR. --> --------- Co-authored-by: Mateusz Kopciński <mateusz.kopcinski@swmansnion.com>
## Description This PR is not ready for merging, I'd like to start a small discussion about C++ codebase. This PR is trying to introduce unified nested-namespace usage strategy across C++ code. However, there is still room for improvement, and I want to ask some questions about how we could improve the structure of whole project. ### Introduced changes: - Previously existing namespaces were renamed from `flatcase` into `snake_case`. For example `rnexecutorch::imageprocessing` -> `rnexecutorch::image_processing`. Change was made according to [Google C++ Style Guide.](https://google.github.io/styleguide/cppguide.html#Namespace_Names) - The biggest directory, i.e `models`, has now namespace usage matching it's directory structure. Each file in there is in `rnexecutorch::models::model_name` namespace. If necessary (i.e always when they exist) more nested namespaces are introduced e.g. `rnexecutorch::models::ocr::utils` or `rnexecutorch::models::classification::constants` or `rnexecutorch::models::object_detection::types`. - **Pros** of the change: - The logical separation of the segments are well reflected by the namespace separation. - It is clearer, whether something is a utility function, something is a type or something Is a constant. - There are no ambiguities, e.g. earlier in the `JsiConversions.h` when referring to `Detection` it was hard to distinguish between type `Detection` returned by OCR and returned by Object Detection. - **Cons** of the change: - Sometimes the nested namespaces are very long. For example having to write `models::object_detection::types::Detection` in `JsiConversions.h` seems a bit redundant. - Maybe our codebase is not that big. Maybe introducing so much separation is an *overkill*. - Having said that, I have to mention, that long nested-namespaces are not usually a problem. If something is in the namespace `rnexecutorch::models::ocr::utils` it is designed to be used by OCR, so probably in the namespace `rnexecutorch::models::ocr`. Adding only `utils::` or `constants::` prefix is not that big of a deal. There are a few cases, as mentioned `JsiConversions.h` when we use type (or even once coco labels constant, but I think it is actually a mistake, it needs refactoring as it should not be there) and then nested-namespaces are painful, but I think it is a rare situation. Maybe the idea of namespaces are okay, but the depth is unnecessary? Maybe we should stop after model-specific namespaces? Sometimes it is a bit weird, introducing the `rnexecutorch::models::object_detection::types` just for one struct. However, I am open to discussion. - Having introduced the `constants::` namespaces I felt like earlier discussion about k-prefix vs UPPER_CASE constants naming (See #515 ) could be resolved differently. Since every constant is already prefixed with `constants::` namespace the UPPER_CASE seems like to much clutter and is not clearer any more. Therefore I changed the constants naming to k-prefix Hungarian Notation according again to [Google C++ Style Guide.](https://google.github.io/styleguide/cppguide.html#Constant_Names) - I had to modify slightly the `REGISTER_CONSTRUCTOR` macro from `metaprogramming` because the forward-declaration does not allow nested namespace symbols. In the end, I moved registering constructors directly to `models/some_model/some_model.h` Previously registering constructors was done in `rnExecutorchInstaller.h` file but looking at the file strcucture I think there were no good reasons for it to happen there. ### Posed questions - Maybe we could introduce more directory-structure-like namespaces? For example right now the namespaces in `data_processing` directory are not connected, we have `rnexecutorch::image_processing`, `rnexecutorch::file_utils`, `rnexecutorch::dsp` and `rnexecutorch::numerical`. Maybe it all could be somehow refactored? My lovely friend Gemini proposed something like `media` directory with namespaces than like `media::audio`, `media::image` (but then `numerical` and `file_utils` should be moved somewhere else), but I am not sure if I like this approach. - `TokenizerModule .h /.cpp` is currently in the very high `rnexecutorch` directory. It is right next to the `RnExecutorchInstaller.h` file. Seems way off, it has to be moved somewhere, but not really into the `models` dir. Maybe the solution is to create new `Tokenizer` dir? It is the only thing apart from the models, that we export to js. Maybe it should be somehow visible in the structure (like directory with models and tokenizers directories inside?). - I do not like the `EncoderDecoderBase` files being at the same height as `BaseModel`. For example `BaseEmbeddings` are at different directory depth, and the structure of `embeddings` directory feels better. How to handle this? - currently things that are related to jsi are mostly in the plain `rnexecutorch::` namespace. The exception is `JsiConversions.h` that lives in the `rnexecutroch::jsi_conversions` namespace (but it is actually in `host_object` directory, while we have `jsi` directory elsewhere.) How to refactor this? - ### Introduces a breaking change? - [ ] Yes - [x] No ### Type of change - [ ] Bug fix (change which fixes an issue) - [ ] New feature (change which adds functionality) - [ ] Documentation update (improves or adds clarity to existing documentation) - [x] Other (chores, tests, code style improvements etc.) ### Tested on - [x] iOS - [ ] Android ### Testing instructions <!-- Provide step-by-step instructions on how to test your changes. Include setup details if necessary. --> ### Screenshots <!-- Add screenshots here, if applicable --> ### Related issues Closes #501 ### Checklist - [x] I have performed a self-review of my code - [x] I have commented my code, particularly in hard-to-understand areas - [ ] I have updated the documentation accordingly - [x] My changes generate no new warnings
…eplace it. (#512) After the port to C++, the static `BaseModule` in typescript is no longer needed. Additionally `NonStaticBaseModule` can take its place and use simpler, less confusing name. This way, in most cases the class that inherits after `BaseModule` corresponds nicely with C++ class that inherits after `BaseModel` C++ class. The same reasoning could be made, and therefore was applied to hooks: `useModule` and `useNonStaticModule` - [ ] Yes - [x] No - [ ] Bug fix (change which fixes an issue) - [ ] New feature (change which adds functionality) - [ ] Documentation update (improves or adds clarity to existing documentation) - [x] Other (chores, tests, code style improvements etc.) - [x] iOS - [ ] Android <!-- Provide step-by-step instructions on how to test your changes. Include setup details if necessary. --> <!-- Add screenshots here, if applicable --> Closes #500 - [x] I have performed a self-review of my code - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] I have updated the documentation accordingly - [x] My changes generate no new warnings <!-- Include any additional information, assumptions, or context that reviewers might need to understand this PR. -->
## Description Adds word `probs` to cspell to be recognized and renames two variables to avoid naming clash inside a function. ### Introduces a breaking change? - [ ] Yes - [x] No ### Type of change - [ ] Bug fix (change which fixes an issue) - [ ] New feature (change which adds functionality) - [ ] Documentation update (improves or adds clarity to existing documentation) - [x] Other (chores, tests, code style improvements etc.) ### Tested on - [x] iOS - [x] Android ### Testing instructions Check if warnings are not present ### Screenshots <!-- Add screenshots here, if applicable --> ### Related issues #536 ### Checklist - [x] I have performed a self-review of my code - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] I have updated the documentation accordingly - [x] My changes generate no new warnings ### Additional notes <!-- Include any additional information, assumptions, or context that reviewers might need to understand this PR. -->
## Description As in the title, some refactor of jsi. ### Introduces a breaking change? - [ ] Yes - [x] No ### Type of change - [ ] Bug fix (change which fixes an issue) - [ ] New feature (change which adds functionality) - [ ] Documentation update (improves or adds clarity to existing documentation) - [x] Other (chores, tests, code style improvements etc.) ### Tested on - [ ] iOS - [ ] Android ### Testing instructions Needs to be manually testes / unit test (TODO) ### Screenshots <!-- Add screenshots here, if applicable --> ### Related issues <!-- Link related issues here using #issue-number --> ### Checklist - [ ] I have performed a self-review of my code - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] I have updated the documentation accordingly - [ ] My changes generate no new warnings ### Additional notes <!-- Include any additional information, assumptions, or context that reviewers might need to understand this PR. -->
) the c++ 20 `std::format` relies on c++ 17 `std::to_char` which was introduced later in libc++ effectively meaning that no build with target < iOS 16.3 can use `std::format`, (see [this](https://developer.apple.com/xcode/cpp/#c++17)) The simplest solution is to basically remove all `std::format` for now. The more future-realistic solutions could be considered. To see the more verbose discussion go to #570. - [ ] Yes - [x] No - [x] Bug fix (change which fixes an issue) - [ ] New feature (change which adds functionality) - [ ] Documentation update (improves or adds clarity to existing documentation) - [ ] Other (chores, tests, code style improvements etc.) - [x] iOS - [ ] Android <!-- Provide step-by-step instructions on how to test your changes. Include setup details if necessary. --> <!-- Add screenshots here, if applicable --> <!-- Link related issues here using #issue-number --> - [x] I have performed a self-review of my code - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] I have updated the documentation accordingly - [x] My changes generate no new warnings <!-- Include any additional information, assumptions, or context that reviewers might need to understand this PR. -->
## Description remove comma causing failing builds on android ### Introduces a breaking change? - [ ] Yes - [x] No ### Type of change - [x] Bug fix (change which fixes an issue) - [ ] New feature (change which adds functionality) - [ ] Documentation update (improves or adds clarity to existing documentation) - [ ] Other (chores, tests, code style improvements etc.) ### Tested on - [x] iOS - [x] Android ### Testing instructions <!-- Provide step-by-step instructions on how to test your changes. Include setup details if necessary. --> ### Screenshots <!-- Add screenshots here, if applicable --> ### Related issues <!-- Link related issues here using #issue-number --> ### Checklist - [ ] I have performed a self-review of my code - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] I have updated the documentation accordingly - [ ] My changes generate no new warnings ### Additional notes <!-- Include any additional information, assumptions, or context that reviewers might need to understand this PR. --> Co-authored-by: Mateusz Kopciński <mateusz.kopcinski@swmansnion.com>
## Description This PR updates static libraries(.a) files for iOS, so they support iOS in version 16 and newer, it also solves the problem with crashes on iPads. Also missing libkernels_portable was added. I suggest to wait with this PR until all features are rewritten to CPP and then also remove `s.ios.vendored_frameworks` line from podspec as it won't be needed anymore, and currently it overwrites linked static libraries ### Type of change - [x] Bug fix (non-breaking change which fixes an issue) - [ ] New feature (non-breaking change which adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected) - [ ] Documentation update (improves or adds clarity to existing documentation) ### Tested on - [x] iOS - [ ] Android ### Testing instructions <!-- Provide step-by-step instructions on how to test your changes. Include setup details if necessary. --> ### Screenshots <!-- Add screenshots here, if applicable --> ### Related issues <!-- Link related issues here using #issue-number --> ### Checklist - [x] I have performed a self-review of my code - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] I have updated the documentation accordingly - [x] My changes generate no new warnings ### Additional notes <!-- Include any additional information, assumptions, or context that reviewers might need to understand this PR. -->
mkopcins
force-pushed
the
@mkopcins/v0.5.3
branch
from
e28e0f2 to
11e2853
Compare
NorbertKlockiewicz
approved these changes
Sep 3, 2025
mkopcins
added a commit
that referenced
this pull request
Oct 15, 2025
## Description <!-- Provide a concise and descriptive summary of the changes implemented in this PR. --> ### Introduces a breaking change? - [x] Yes - [ ] No ### Type of change - [x] Bug fix (change which fixes an issue) - [x] New feature (change which adds functionality) - [ ] Documentation update (improves or adds clarity to existing documentation) - [ ] Other (chores, tests, code style improvements etc.) ### Tested on - [x] iOS - [x] Android ### Testing instructions <!-- Provide step-by-step instructions on how to test your changes. Include setup details if necessary. --> ### Screenshots <!-- Add screenshots here, if applicable --> ### Related issues <!-- Link related issues here using #issue-number --> ### Checklist - [ ] I have performed a self-review of my code - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] I have updated the documentation accordingly - [ ] My changes generate no new warnings ### Additional notes <!-- Include any additional information, assumptions, or context that reviewers might need to understand this PR. --> --------- Co-authored-by: Mateusz Kopciński <mateusz.kopcinski@swmansnion.com> Co-authored-by: Filip Zieliński <127685479+mlodyjesienin@users.noreply.github.com> Co-authored-by: Mateusz Sluszniak <56299341+msluszniak@users.noreply.github.com> Co-authored-by: Norbert Klockiewicz <Nklockiewicz12@gmail.com>
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
4 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
Introduces a breaking change?
Type of change
Tested on
Testing instructions
Screenshots
Related issues
Checklist
Additional notes