We could use std::unordered_map over std::map by Fabio3rs · Pull Request #305 · ggml-org/llama.cpp

Fabio3rs · 2023-03-19T21:52:07Z

If it is not necessary sorted maps, change std::map to std::unordered_map

std::unordered_map is a hash table so it should be faster than std::map when storing many items.

std::map<id, token> can be a std::vector since the vector index can be equal to the token id

…d::map<id, token> id_to_token; to std::vector<token> id_to_token;

…token_to_id.size());

main.cpp

utils.h

eiz · 2023-03-20T10:26:21Z

the token vector should prob be a struct now which also includes the score (see 074bea2)

Fabio3rs · 2023-03-20T12:43:20Z

the token vector should prob be a struct now which also includes the score (see 074bea2)

I did a commit merging with the new changes using struct, I am not sure about the names or the organization of the struct

z11h · 2023-03-20T22:13:03Z

Are you able to measure the performance gains from these changes? Interested to see how much of an impact they have.

Great work!

Fabio3rs · 2023-03-21T00:16:10Z

Are you able to measure the performance gains from these changes? Interested to see how much of an impact they have.

Great work!

Thanks!

Sadly it's hard to do consistent tests, but what I got is:
last commit (bd4b46d) result with: ./build/llama -m models/7B/ggml-model-f16.bin --color -p 'tdd is' --seed 0

main: mem per token = 14696644 bytes
main:     load time =  3236.45 ms
main:   sample time =    57.25 ms
main:  predict time = 37547.44 ms / 286.62 ms per token
main:    total time = 41166.49 ms

last commit (bd4b46d) + this pull request: (same command above)

main: mem per token = 14696644 bytes
main:     load time =  3204.60 ms
main:   sample time =    57.64 ms
main:  predict time = 37372.86 ms / 285.29 ms per token
main:    total time = 40956.10 ms

This differences should be more noticeable with larger datasets and more tokens, I am using the model 7B.

Green-Sky · 2023-03-21T00:18:44Z

Are you able to measure the performance gains from these changes? Interested to see how much of an impact they have.

there should not really be any. none of the code is particularly hot.

Green-Sky · 2023-03-21T00:22:28Z

utils.h

 // Vocab utils
 //

+struct token_score {


this is confusingly named, same with token_t. the type is only used inside gpt_vocab, so why not nest it.

also gpt_vocab is token_t already in this case

Thanks!

What name can I give this struct?

hm, first thought was token_t, but that is too close to token, so, just leave it as token_score.

This compiler version seems to not accept token token;

/home/runner/work/llama.cpp/llama.cpp/utils.h:68:15: error: declaration of ‘gpt_vocab::token gpt_vocab::token_score::token’ changes meaning of ‘token’ [-fpermissive] 68 | token token; | ^~~~~ /home/runner/work/llama.cpp/llama.cpp/utils.h:65:11: note: ‘token’ declared here as ‘using token = std::string’ 65 | using token = std::string;

Should I rename the using token = std::string; to token_t?

The quickest and simplest fix to that would be to just rename the data member to tok.

ggerganov · 2023-03-21T17:01:37Z

Apologies for the conflicts - lets resolve and merge.

Regarding the C++ standard question from the other thread:
There are a few reasons and I know there will be people that will disagree and I totally understand. I just think this way it is less incentivising to use overly-complicated constructs. You are correct that in this case we can actually gain some performance with std::string_view and even save unnecessary allocations, but this part of the code is totally negligible compared to the full transformer evaluation. Plus, we can do the suggested change using raw pointers - yes, bit more ugly, but the performance will be there.

Overall, my experience tells me this is the better way - or at least it is better in my views and understandings. And if there ever appears a very good reason to bump the standard - we will do it. But at the moment, there is no good enough reason to do it.

Fabio3rs · 2023-03-21T17:14:13Z

Apologies for the conflicts - lets resolve and merge.

Regarding the C++ standard question from the other thread: There are a few reasons and I know there will be people that will disagree and I totally understand. I just think this way it is less incentivising to use overly-complicated constructs. You are correct that in this case we can actually gain some performance with std::string_view and even save unnecessary allocations, but this part of the code is totally negligible compared to the full transformer evaluation. Plus, we can do the suggested change using raw pointers - yes, bit more ugly, but the performance will be there.

Overall, my experience tells me this is the better way - or at least it is better in my views and understandings. And if there ever appears a very good reason to bump the standard - we will do it. But at the moment, there is no good enough reason to do it.

Thanks!

I think I resolved the conflicts, if there are some problems I am happy to fix.

Fabio3rs added 2 commits March 19, 2023 18:43

Improve performance by changing std::map to std::unordered_map and st…

25ef27c

…d::map<id, token> id_to_token; to std::vector<token> id_to_token;

fix last commit on gpt_vocab_init add vocab.id_to_token.resize(vocab.…

78b964e

…token_to_id.size());

Fabio3rs changed the title ~~If it is not necessary sorted maps, change std::map to std::unordered_map~~ Use unordered_map over std::map Mar 19, 2023

Fabio3rs changed the title ~~Use unordered_map over std::map~~ Use std::unordered_map over std::map Mar 19, 2023

Green-Sky suggested changes Mar 19, 2023

View reviewed changes

main.cpp Show resolved Hide resolved

utils.h Outdated Show resolved Hide resolved

Removed include <map>

40ab486

Green-Sky approved these changes Mar 19, 2023

View reviewed changes

Merge unordered_map/vector changes with trunk updates

ef792ae

Fabio3rs changed the title ~~Use std::unordered_map over std::map~~ We could use std::unordered_map over std::map Mar 20, 2023

gjmulder added the enhancement New feature or request label Mar 20, 2023

Green-Sky suggested changes Mar 21, 2023

View reviewed changes

Nest struct token score inside gpt_vocab

3459653

Fabio3rs requested a review from Green-Sky March 21, 2023 12:36

renamed token to tok

a19aa63

Fabio3rs mentioned this pull request Mar 21, 2023

Add tokenizer test + revert to C++11 #355

Merged

Resolved recent conflicts with master

cfdf363

ggerganov merged commit 353ec25 into ggml-org:master Mar 21, 2023

Conversation

Fabio3rs commented Mar 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

eiz commented Mar 20, 2023

Uh oh!

Fabio3rs commented Mar 20, 2023

Uh oh!

z11h commented Mar 20, 2023

Uh oh!

Fabio3rs commented Mar 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Green-Sky commented Mar 21, 2023

Uh oh!

Green-Sky Mar 21, 2023

Choose a reason for hiding this comment

Uh oh!

Green-Sky Mar 21, 2023

Choose a reason for hiding this comment

Uh oh!

Fabio3rs Mar 21, 2023

Choose a reason for hiding this comment

Uh oh!

Green-Sky Mar 21, 2023

Choose a reason for hiding this comment

Uh oh!

Fabio3rs Mar 21, 2023

Choose a reason for hiding this comment

Uh oh!

feroldi Mar 21, 2023

Choose a reason for hiding this comment

Uh oh!

ggerganov commented Mar 21, 2023

Uh oh!

Fabio3rs commented Mar 21, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Fabio3rs commented Mar 19, 2023 •

edited

Loading

Fabio3rs commented Mar 21, 2023 •

edited

Loading