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

Closed
wants to merge 2 commits into 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

@skongum02 skongum02 deleted the branch modular:nightly January 29, 2025 18:58
@skongum02 skongum02 closed this Jan 29, 2025
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.

4 participants