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

Use indexmap for TxtProperties #300

Closed
Luni-4 opened this issue Feb 4, 2025 · 4 comments
Closed

Use indexmap for TxtProperties #300

Luni-4 opened this issue Feb 4, 2025 · 4 comments

Comments

@Luni-4
Copy link

Luni-4 commented Feb 4, 2025

I was just looking at these code lines and I've thought that the indexmap collection would be efficient for TxtProperties's use-cases.

Advantages:

  • Efficient because a TxtProperty cannot be removed from the collection, but only read. This would always respect the insertion order. I've based my assumption from the methods provided by this structure though
  • Fast iteration
  • On par with HashMap in terms of performance

Disadvantages:

  • A new dependency
  • The dependency might be unmaintained one day

It would also be helpful to provide a method to access the internal indexmap

@keepsimple1
Copy link
Owner

Something like indexmap would be indeed more efficient than Vec<> used today. However, I think in this particular case, the downside (as you also mentioned) overweights the upside. One reason is that TxtProperties mostly has just a few items, hence the difference between using Vec and indexmap would be small. On the other hand, fewer dependencies is a good thing to keep.

If any real world use case (or measurements) shows that TxtProperties is a performance bottleneck, then we can look into it more.

@Luni-4
Copy link
Author

Luni-4 commented Feb 5, 2025

I agree with your analysis. The number of TxtProperty is usually low, hence the performance impact remains small independently of the collection in use.

As personal use-case, I need to convert TxtProperties into HashMap<String, String> in order to save them in memory and retrieve the information more easily, so the use of IndexMap would have been simpler.

As it is now, we need to allocate Strings from &str returned by the val_str() method. Perhaps, adding a method which consumes TxtProperties into an HashMap<String, String> would be the most suitable solution for my use-case.

Here an example of how I'm doing that right now.

info.get_properties()
    .iter()
    .map(|v| (v.key().to_string(), v.val_str().to_string()))
    .collect()

@keepsimple1
Copy link
Owner

Perhaps, adding a method which consumes TxtProperties into an HashMap<String, String> would be the most suitable solution for my use-case.

I think we can add a new method for such cases. I've opened a draft PR #303 based on my understanding. Please let me know if that would work for you.

@keepsimple1
Copy link
Owner

PR merged. Thanks!

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