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.

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

[stdlib] fix: remove DictEntry.__copyinit__ in Dict methods (Pointer, speed up) #3824

Open
wants to merge 2 commits into
base: nightly
Choose a base branch
from

Conversation

rd4com
Copy link
Contributor

@rd4com rd4com commented Nov 28, 2024

Hello, this probably fixes some bug reports 🥳
it is also now faster because we use Pointer and it removes __copyinit__

Many methods like __getitem__ uses _find_index,
it was doing __copyinit__ on entries while iterating

The result is also a speedup, especially on types that allocate memory,
best speedup is probably for V:Arc 👍

It should also speedup **kwargs!

@rd4com rd4com requested a review from a team as a code owner November 28, 2024 22:12
@rd4com
Copy link
Contributor Author

rd4com commented Nov 28, 2024

🔥 benchmarks

Theses are some small simple benchmarks with Time.now(),
speedups increases with the size of the dictionary ❤️‍🔥

690781 3264000 (1.13x)
885908 6528000 (1.19x)
2173504 6593800 (3.36x)
1645963 6659600 (2.32x)
316705 6725400 (7.3x)
from time import now
from collections import Dict

alias iteration_size = 1<<8
def main():
    var result: Int=0
    var start = now()
    var stop = now()

    
    small = Dict[Int, Int]()
    start = now()
    for x in range(100):
        for i in range(iteration_size):
            small[i]=i
        for i in range(iteration_size): 
            result += small.pop(i)
    stop = now()
    print(stop-start, result)
    
    start = now()
    for x in range(100):
        small = Dict[Int, Int]()
        for i in range(iteration_size):
            small[i]=i
        for i in range(iteration_size): 
            result += small[i]
    stop = now()
    print(stop-start, result)
    
    start = now()
    for x in range(100):
        small2 = Dict[Int, String]()
        @parameter
        for i in range(iteration_size):
            alias tmp = str(i)
            small2[i]=tmp
        for i in range(iteration_size): 
            result += len(small2[i])
    stop = now()
    print(stop-start, result)
    
    small2 = Dict[Int, String]()
    start = now()
    for x in range(100):
        @parameter
        for i in range(iteration_size):
            alias tmp = str(i)
            small2[i]=tmp
        for i in range(iteration_size): 
            result += len(small2.pop(i))
    stop = now()
    print(stop-start, result)
    
    small2 = Dict[Int, String]()
    @parameter
    for i in range(iteration_size):
        alias tmp = str(i)
        small2[i]=tmp
    start = now()
    for x in range(100):
        for i in range(iteration_size): 
            result += len(small2[i])
    stop = now()
    print(stop-start, result)

@msaelices
Copy link
Contributor

This is awesome!!!

Copy link
Collaborator

@JoeLoser JoeLoser left a comment

Choose a reason for hiding this comment

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

Nice work!

stdlib/test/collections/test_dict.mojo Outdated Show resolved Hide resolved
@JoeLoser JoeLoser self-assigned this Dec 4, 2024
…nter`, speed up)

Hello, this probably fixes some bug reports 🥳
it is also now faster because we use `Pointer` and it removes `__copyinit__`
(
    Many methods like `__getitem__` uses `_find_index`,
    it was doing `__copyinit__` on entries while iterating
)

The result is also a speedup, especially on types that allocate memory,
best speedup is probably for `V:Arc` :thumbsup

It should also speedup `**kwargs`!

Signed-off-by: rd4com <[email protected]>
@rd4com
Copy link
Contributor Author

rd4com commented Dec 4, 2024

Thanks !

✅ Changes implemented

  • [review] remove iter for test_dict_entries_del_count()
  • rebase

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