Skip to content
This repository was archived by the owner on Jan 8, 2025. It is now read-only.

dev: optimize conversion of bytes to words #940

Closed
enitrat opened this issue Sep 13, 2024 · 7 comments · Fixed by #985
Closed

dev: optimize conversion of bytes to words #940

enitrat opened this issue Sep 13, 2024 · 7 comments · Fixed by #985
Assignees
Labels
enhancement New feature or request ODHack8

Comments

@enitrat
Copy link
Contributor

enitrat commented Sep 13, 2024

Some benchmarks:

pub fn ceil32(value: usize) -> usize {
    let ceiling = 32_u32;
    let (_q, r) = DivRem::div_rem(value, ceiling.try_into().unwrap());
    if r == 0_u8.into() {
        return value;
    } else {
        return (value + ceiling - r).into();
    }
}

#[inline(never)]
pub fn words_size(value: usize) -> usize {
    return (value + 31) / 32;
}

#[inline(never)]
pub fn words_size_ceil(size: usize) -> u64 {
    let words_size = (ceil32(size) / 32).into();
    return words_size;
}

#[cfg(test)]
mod tests{
    use super::{words_size, words_size_ceil};

    #[test]
    fn test_words_size(){
        let x = words_size(1);
        let x = words_size(32);
        let x = words_size(33);
    }

    #[test]
    fn test_words_size_ceil(){
        let x = words_size_ceil(1);
        let x = words_size_ceil(32);
        let x = words_size_ceil(33);
    }

}
image image

It's almost twice cheaper to to (bytes+31) / 32 then to (ceil32(bytes) /32) to compute the amount of 32bytes words required to represent size bytes.

  • Locate all ceil32(size) usage throughout the codebase
  • Replace with a function 32bytes_words_size marked as #[inline(always)]
@enitrat enitrat added the enhancement New feature or request label Sep 13, 2024
@github-project-automation github-project-automation bot moved this to 🆕 Backlog in Kakarot on Starknet Sep 13, 2024
@mubarak23
Copy link

mubarak23 commented Sep 26, 2024

I am applying to this issue via OnlyDust platform.

My background and how it can be leveraged

I am a Experience Cairo smart contract developer with experience working on projects such as Just Art Peace, Dojo, Kart, TBA, and Shinigami. Before transitioning to Cairo development, I was a backend developer specializing in Rust.

My recent work with cairo starknet

My recent work with rust

How I plan on tackling this issue

I estimate this will take 16hrs to complete.

This is how I would tackle this issue:

  • Locating all ceil32(size) in the codebase
  • Replace it with 32bytes_words_size function
  • Make sure scarb build successfully
  • Write unit test where needed

ETH: 16HRS

@lordshashank
Copy link
Contributor

I am applying to this issue via OnlyDust platform.

My background and how it can be leveraged

Hi, I have been contributing in odhacks for quite some time now and have expertise in rust and blockchain specs , few of them are

  • Contributed to Floresta as Summer of Bitcoin 2024 fellow
  • contributed to RUst lang's codegen GCC and implemented new traits and tests.
  • Contributed to starknet rust devnet in odhack

How I plan on tackling this issue

I will replace all ceil32(size) with bytes parallel and integrate this with codebase
I will ensure that all tests work well to avoid breaking thigs due to this change

@guha-rahul
Copy link

I am applying to this issue via OnlyDust platform.

My background and how it can be leveraged

I am a cairo dev and and this can be leverged in this issue.

How I plan on tackling this issue

This is a much simplier issue to solve so we need to locate all the ceil32(size) and replace them with a much cheater implementation

@TropicalDog17
Copy link

Hi @enitrat may I ask how do you get the benchmark image?

@od-hunter
Copy link

I am applying to this issue via OnlyDust platform.

My background and how it can be leveraged

Please can I be assigned this issue? This would be my first time contributing to this ecosystem and I’d love to be given the opportunity. I am a blockchain Developer, and my experience includes html, css, react, JavaScript,TypeScript and solidity, python and Cairo.

How I plan on tackling this issue

To solve this issue, I’d take the following steps:
1.⁠ ⁠I’ll first create the New Function, that is, I’ll implement bytes_to_32words(size: usize) using the formula (size + 31) / 32, and mark it with #[inline(always)] for optimization.
2.⁠ ⁠⁠Next, I’ll search the codebase for instances of ceil32(size).
3.⁠ ⁠⁠I’ll substitute each occurrence of ceil32(size) with bytes_to_32words(size) to maintain functionality.
4.⁠ ⁠⁠Then, I’ll execute all tests to ensure that the changes do not introduce regressions. ( I can also benchmark the code before and after changes to evaluate performance improvements).

Please assign me. I'm ready to work.

@martinvibes
Copy link

I am applying to this issue via OnlyDust platform.

My background and how it can be leveraged

hello i am a frontend dev and blockchain developer
please can i work on this issue :) and would love to be a contributor

How I plan on tackling this issue

Analyze Existing Functions: Review the ceil32 and words_size functions for understanding.
Identify Inefficiencies: Note that (bytes + 31) / 32 is more efficient than using ceil32(bytes).
Locate Usage: Search for instances of ceil32(size) in the codebase.
Implement Replacement Function: Create a new function 32bytes_words_size marked with #[inline(always)] for the optimized calculation.
Replace Old Calls: Substitute all instances of ceil32(size) with the new function.
Test the Changes: Ensure existing tests cover the new function and add new tests if necessary.
Run Benchmarks: Compare performance before and after the optimization.
Commit Changes: Save changes with a clear commit message about the optimization.

Copy link

onlydustapp bot commented Sep 26, 2024

The maintainer enitrat has assigned lordshashank to this issue via OnlyDust Platform.
Good luck!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request ODHack8
Projects
No open projects
Archived in project
Development

Successfully merging a pull request may close this issue.

7 participants