Skip to content

Conversation

Parizval
Copy link

As per the Issue #344, Computing compression ratio is skipped for L1 messages.

// Note: We compute the transaction ratio on tx.data, not on the full encoded transaction.
let compression_ratio = compute_compression_ratio(base.input());
Self::new(base, encoded, Some(compression_ratio))
let compression_ratio =

Choose a reason for hiding this comment

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

then_some is eagerly evaluated. Better to use then to avoid the computation altogether for L1 messages. (Or just a simple if expression.)

Copy link
Collaborator

Choose a reason for hiding this comment

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

nice catch!

Copy link
Author

Choose a reason for hiding this comment

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

Should I refactor both the values (encoded and compression_ratio) ? @greged93 @Thegaram

Copy link
Collaborator

Choose a reason for hiding this comment

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

encoded is ok, since it's just a reference to a var, it's fine to evaluate it eagerly.

@Thegaram Thegaram requested a review from greged93 September 26, 2025 07:38
Copy link
Collaborator

@greged93 greged93 left a comment

Choose a reason for hiding this comment

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

update with FnOnce and should be good!

// Note: We compute the transaction ratio on tx.data, not on the full encoded transaction.
let compression_ratio = compute_compression_ratio(base.input());
Self::new(base, encoded, Some(compression_ratio))
let compression_ratio = (!tx.is_l1_message()).then(compute_compression_ratio(base.input()));
Copy link
Collaborator

Choose a reason for hiding this comment

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

then takes a FnOnce, you need to add ||

// Note: We compute the transaction ratio on tx.data, not on the full encoded transaction.
let compression_ratio = compute_compression_ratio(base.input());
Self::new(base, encoded, Some(compression_ratio))
let compression_ratio = (!tx.is_l1_message()).then(compute_compression_ratio(base.input()));
Copy link
Collaborator

Choose a reason for hiding this comment

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

same

Copy link

codspeed-hq bot commented Sep 26, 2025

CodSpeed Performance Report

Merging #345 will not alter performance

Comparing Parizval:skip-compression (337c392) with scroll (d590bf0)

Summary

✅ 77 untouched

@Parizval Parizval requested a review from greged93 September 26, 2025 13:10
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.

3 participants