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

wrong size of elements? #3

Open
nikooo777 opened this issue Nov 20, 2020 · 4 comments
Open

wrong size of elements? #3

nikooo777 opened this issue Nov 20, 2020 · 4 comments

Comments

@nikooo777
Copy link

Hello there,

First of all, thank you so much for this nice implementation of LFUDA! I read your post on medium and I really enjoyed it.

I was testing around with it and I noticed that when passing booleans as values of an element, the size of the value turns out to be 4 bytes which is kinda odd given that a boolean is supposed to be just 1 byte.
I did some digging and I found the snippet of code that calculates the size and I wondered if there was any reason I could not see as to why you did it that way.

		// if the value is binary
		if valBytes, ok := value.([]byte); ok {
			numBytes = float64(len(valBytes))
		} else {
			// otherwise use the default format
			numBytes = float64(len([]byte(fmt.Sprintf("%v", value.(interface{})))))
		}

I put together a quick snippet that would compare your implementation with 2 others I pieced together, one seems to be more accurate than the other, however it still fails on at least one example.

Do you have any opinions on what the real solution should be? I'd be very interested in using the library, however I have to get this right.

https://play.golang.org/p/GdkA4azQqZB

Best regards & stay safe!

Niko

@bparli
Copy link
Owner

bparli commented Nov 23, 2020

Hi @nikooo777, I'm glad you find this package useful! You found the roughest part of the code and I'm still not sure on a good, general solution. Binary.Size() works in some cases but I didn't find unsafe to work in most. Reflect.TypeOf was actually similar to binary IIRC.

I pushed PR #4 to include binary.Size for the more complex types and booleans. I feel there must be a better way to do this but still don't see it. Would appreciate if you can have a look and let me know what you think

The []interface{}{x} case is an interesting one, but wouldn't be covered by these updates still unfortunately

@nikooo777
Copy link
Author

hey @bparli

I checked your PR and it looks like a reasonable first step to take. I wasn't able to find anything better out there, but we can't be the only ones having this problem to solve so eventually we'll get it right :)

If you're curious to see this library in use and how it performs I'd gladly loop you in. I implemented it on a project we use to stream content to users and it basically handles evictions from disk based on the observed requests.

I'm experiencing good results with about 850000 (~1.7TB) items tracked through LFUDA (data on nvme) however it gets a bit more interesting when handling 30'000'000 items (almost 60TB) on disk.

My intentions are to investigate the performance issues i'm noticing and possibly improving your library.

@bparli
Copy link
Owner

bparli commented Nov 24, 2020

Hey @nikooo777, thats really great to see you're pushing its limits. For now I'll merge that PR but would be happy to hear more about your findings and collaborate on improvements.

@nikooo777
Copy link
Author

Nice! I'll update it tomorrow and gather some more data. I'll keep you posted! ;)

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

No branches or pull requests

2 participants