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.

Sign up for GitHub

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

Jump to bottom

Epic rewrite #378

Open
wants to merge 29 commits into
base: main
Choose a base branch
from
Open

Epic rewrite #378

wants to merge 29 commits into from
+23,980 −13,209

Conversation

nazmulidris
Copy link
Member

@nazmulidris nazmulidris commented Feb 3, 2025 •
edited
Loading

Started with

  • [core][tui] Clean up emoji use in logs, enhance tracing support
  • [all] Promote scripting.rs from rust_scratch/tls

Also ended up doing

  • Rewrite entire codebase to use smallvec and smallstr
  • Change UnicodeString impl and UnicodeStringSegment
  • Fix edi bugs
  • Change units from ChUnit to strongly typed units for height, width, row, col, dim, and pos.
  • Implement new telemetry system using ringbuffer (no global static or use of atomics)

Issues closed

@nazmulidris
[core][tui] Clean up emoji use in logs, enhance tracing support
4cc1ab9
@nazmulidris
[all][script][log] Promote scripting.rs from rust_scratch/tls
4cb6759 
Create 2 new crates:
- script
- log
  - support UnicodeString for output
  - update all calls to tracing::*() to work seamlessly with CustomEventFormatter
    across the entire r3bl codebase (especially r3bl_tui examples)
  - add of the `options: impl Into<TracingConfig>` pattern for configuring the
    log functionality. this is ergonomic for simple cases (for the caller) and
    provides sophisticated configuration possibilities due to its composability.

Reorganize all the existing crates as well, to ensure that
known duplicated functions are removed.

Add new glyphs that are unicode character based, not emojis,
so that the fg, bg color and attributes are applied in output.
Emojis come with their own colors, so the fg and bg colors don't
get applied.

Use Vec::with_capacity wherever possible to prevent having reallocations
which are expensive, and create a global size limit for the all the r3bl
crates.
@nazmulidris
[all] Use smallvec and smallstr, memory latency, telemetry, performan…
5603296 
…ce tuning

- Data oriented design: https://youtu.be/WwkuAqObplU?si=5JPiDCfAG4YQxlhG
- Memory alloc: https://youtu.be/pJ-FRRB5E84?si=EGD_Tge1ApGRkT6n&t=1831
- Scalarize/flatten: https://youtu.be/Dhn-JgZaBWo?si=oFiWnwKEhfJFxZCL

Refactor ch_unit.rs:
- Remove `ch!` and replace it easier to understand functions that
  are easier to compose, and produce better compiler errors.

Change the memory usage of the codebase:
- Preallocate as much as possible.
- Allocate on the stack when possible.
- Reduce the size of fields if they don't need to store large `usize`
  values.

Update telemetry_global_static to provide MAX FPS

Implement RingBuffer and RateLimiter

Add support for `$TERM_PROGRAM` equals `vscode` to enable truecolor

Add TimeDuration struct to format Duration in a performant
and human readable way. No memory is allocated in the Display
trait implementation (the function that renders `{}`).

Update all Cargo.toml to Rust edition 2024

Remove `static_global_data.rs` module. Telemetry is now handled via the
`Telemetry` struct, and `RateLimiter` and `RingBuffer`.

Add rendering hints to Telemetry along with the telemetry_record!
macro to make the telemetry measurement ergonomic and self
documenting

Clean up log output to use tracing::debug!, etc. correctly
with message = {message}, etc.
@nazmulidris
[all] Continue memory latency performance tuning work
57ff54b 
[ At the start ... ]

This is the 2 week long refactor that is showing performance
regression over the previous commit!

excluding the following:
- tui/md_parser/convert_to_plain_text.rs
- tui/syntax_highlighting/intermediate_types.rs
- tui/syntax_highlighting/md_parser_syn_hi/md_parser_syn_hi_impl.rs

The files in this commit had to be rolled back as part of the changes
made in the last commit were breaking all the tui tests, especially
around MD parsing. These files still need the memory optimization work:
- stack vs heap app
- use write! instead of format!

[ And then ... ]

This commit brings the performance back up to previous commit with
SHA: 5603296

The slow down, as far as I can tell, from running different
commits side by side, is that the computation of the UnicodeString
proved to be quite expensive to do every time. So it is beneficial
to memoize this! Running the code with flamegraph profiling didn't
really show anything useful related to this problem.

Apply heap allocation techniques to the code in the following modules
to boost performance and keep it stable (steady):
- [x] formatter.rs
- [x] editor_engine.rs
- [x] convert_to_plain_text.rs
- [x] intermediate_types.rs
- [x] md_parser_syn_hi_impl.rs
@nazmulidris
[tui] Simplify editor buffer post mutation validation
09ccbe1 
Remove the complexity of having an editor_engine_internal_api.rs apply_changes()
function that accepts a closure by using a Drop impl on the struct returned by
editor_buffer_struct.rs get_mut().

Also fix this bug:
#343

Closes #343
@nazmulidris nazmulidris force-pushed the epic-rewrite branch 5 times, most recently from e091ab3 to 8372ca9 Compare February 3, 2025 15:26
@nazmulidris
[tui] Clean up confusing caret types - raw and scroll adjusted
c60209f 
Caret position can be of two kinds:
- Raw
- Scroll adjusted

Make this explicity using the type system and provide conversions
between the two.

Introduce new concrete types for:
- Pos
  - row
  - col
- Dim
  - width
  - height
- Use them in the codebase instead of ChUnit.
- Use ChUnit as the "inner" type of these new concrete types.
@nazmulidris nazmulidris force-pushed the epic-rewrite branch 3 times, most recently from 7baa9d6 to 688a641 Compare February 13, 2025 11:52
@nazmulidris
[tui] Rename, reorganize src/tui/editor/ modules
99120f6 
- Reorganize & create new modules for engine and buffer
- Extract more modules that are distinct semantically
- Remove empty structs used as namespace, and replace with modules
- Remove Remove needless -> Option<()> statements
@nazmulidris
[tui][tuify] Replace bool with enums for better readability
5d3dc60
@nazmulidris
[log] Use ordermap crate instead of custom ordered_map.rs
670e4b2
@nazmulidris
[tui] Clean up sanitize_and_save_abs_position()
5e58eca 
- Clean up log messages
- Clean up imports
- Add docs
- Add window size display when demo starts
@nazmulidris
[all] Add workflow files
d9a8ee1 
- .code-search files are to keep track of work that is pending
- scratch.rs is a file with snippets that are useful
@nazmulidris
[tui] Fix sanitize_and_save_abs_pos()
cc9e6f0 
There was a bug in the code dating back to when this function
was first written. This bug didn't really affect anything. However
it got exposed when the conversion to "newtype" pattern was introduced
and all the ChUnit types got replaced with Pos, Dim, Width, Height, etc.

Make variable names shorter, so that they don't have the "intended use
in the absence of concrete types" backed into the their extremely long
and unreadable names.

This is all thanks to the work related to the "newtype" pattern!
@nazmulidris nazmulidris force-pushed the epic-rewrite branch 2 times, most recently from 0f65d0a to f45f90c Compare February 18, 2025 14:58
@nazmulidris nazmulidris force-pushed the epic-rewrite branch 5 times, most recently from c800be4 to fdbd030 Compare February 18, 2025 20:02
@nazmulidris
[tui] Replace use of bool with enum variants for readability
c93aad3 
Fix bug in main event loop which was preventing the app from handling
resize events. This error was introduced in a previous refactor where
the resize event gets consumed by the main event loop, after it records
telemetry about it; instead of then also giving it to the app to
handle or process.
@nazmulidris
[tui] Use "newtype" pattern in EditorBufferMut
9f8b8f9 
Extract the inner type as EditorBufferMut.

Provide two new types:
1. When validation is needed after mutations, using Drop.
2. When no validation needs to be performed (no Drop impl).
@nazmulidris
[tui] Add tests to validate_scroll_on_resize
95917f6 
After fixing the newly introduced bug where resize events were no
longer passed into the app by main event loop, add missing tests
to ensure that the code works properly. Update diagrams using
nice unicode drawing symbols.

Unicode glyphs links (for the ASCII diagrams):
- https://symbl.cc/en/unicode/blocks/box-drawing/
- https://symbl.cc/en/unicode/blocks/arrows/
- https://symbl.cc/en/collections/brackets/
@nazmulidris nazmulidris force-pushed the epic-rewrite branch 2 times, most recently from 537344b to 29c374f Compare February 19, 2025 18:53
@nazmulidris
[tui] Remove bool from modifier_keys_mask.rs
81a539e 
- Remove redundant type conversions in this module.
- Use `fn foo(arg: into Impl<Struct>)` to make functions more ergonomic
  and reusable.
- Clean up Rust doc comments.
@nazmulidris
[core] Make RingBuffer impl flexible and allow stack and heap storage…
8bf35d2 
… options
@nazmulidris
[core][tui] Minor doc updates
ea91df0
@nazmulidris
[script] Fix test so that it does not hang up when github API is slow
2d1b6ed
@nazmulidris
[core][tui] Change range.rs so that it uses CaretScrAdj type
79bd6b9 
This is more disambiguation by using proper types (now that they exist)
by favoring more specific types over more generalized types in order to
remove the need for documentation or overly long variable names (that
describe the intent, which is now captured in the type system and can
be checked by the compiler).

SelectionRange now only works with CaretScrAdj types, so there's no way
to pass a CaretRaw into the thing. And there's only one way to construct
it. And it removes the RowIndex as part of construction (by clobbering
it with an "impossible" value, rendering it clearly unusable and also
looking strange when debugging, which is the intent).

Clean up the dummy view port use case in buffer_selection_support.rs so
that it no longer passes a 0x0 viewport when it doesn't have one. And
instead passes some "impossible" value, rendering it clearly unusable
and also looking strange when debugging, which is the intent).
@nazmulidris
[script] Make tests that rely on github resilient to outages
bc0f317 
Introduce a 1 sec timeout that will simply skip the test if github API
is taking too long to run. This prevents a github slowdown or outage
disrupting running all tests.
@nazmulidris
[core] Rename range.rs to selection_range.rs
91e064c
@nazmulidris
[tui][core] Move SelectionRange out of core into tui
6938c42 
This struct should not have been in core to begin with. Thanks to the
recent rewrite of the entire crate using "newtype" pattern that favors
specific types over generalized ones, it became clear where this struct
should belong. It wasn't clear before, with generalized type, which is
why it was placed beside UnicodeString in core crate, rather than beside
buffer_selection_support.rs in tui/editor/editor_buffer/ in tui crate.
@nazmulidris
[core][tui] Minor cleanup and doc updates
9ef8cf4
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[tui][edi] Loss of caret focus bugs [tui][edi] Clean up telemetry collection and reporting
1 participant
@nazmulidris