What's the point of SkimItems with non-static lifetime?

[kimono-koans]

What's the point of SkimItems with non-static lifetime? The items are already passed around as Arc<dyn SkimItem> which has a static lifetime anyway, and this breaks the ability to downcast the items (because you can't downcast them), as seen in the downcast example which no longer downcasts the items and instead uses the .text() method from the trait.

Originally posted by @idanarye in #11 (comment)

[kimono-koans]

What's the point of SkimItems with non-static lifetime? The items are already passed around as Arc which has a static lifetime anyway

I believe the idea was to reduce memory usage after a skim invocation, which was a major issue for httm.

One idea was to rip out all static lifetimes everywhere in skim. httm was still holding onto lots of memory after a skim invocation, whereas now it doesn't. I'm sure now much of this was down to the design of my SkimItems.

I suppose strict API compatibility is/was not a goal of mine and downcast was not a feature I was especially interested in or used. two_percent, to the extent I work on it, serves httm. I/it may have been misguided, but FYI my priority was/is memory usage after invocation (which I can also imagine could be an issue for other library users). After tearing much of this out, I came to understand that the 'static lifetime is implicit re: these boxed trait objects, which I probably didn't at the time.

So -- you're not collaborating with a Rust genius (or even a professional programmer!). I took a hacksaw to skim probably because I didn't really understand it and no one else was/is working on it. That means -- feel free to question my decisions. And, if you want downcast and are using it, please feel to add back in a feature branch, so I can test with httm.

[idanarye]

I see that httm only implements SkimItem for SelectionCandidate. Mind if I try parametrizing Skim (and most of the other types, really) to take the concrete type as generic parameter? That should improve performance for everyone.

[kimono-koans]

I see that httm only implements SkimItem for SelectionCandidate. Mind if I try parametrizing Skim (and most of the other types, really) to take the concrete type as generic parameter? That should improve performance for everyone.

Yeah, that'd be great. Give it a shot.

My feeling re: any skim API is: Can we easily refactor our app code? (So maybe a link to how you refactored your own would be good.) Because I'm not sure sk is being developed and used by anyone else. If I or they want to port my/your changes back, that's great, but I don't see anything happening re: skim.

Dozens of things are more important to me, ... like competing with fzf. See: skim-rs#561

fzf is truly impressive but skim should be faster and use less memory, and if we are calling skim as a library, when we return from running skim, I'd like that memory back.

When I say I'm only interested in two_percent re: httm, that isn't exactly true. I also use it as my default fuzzy finder for things like Ctrl+R from zsh. See: https://github.com/kimono-koans/ppa/tree/main, and https://github.com/casonadams/skim.zsh

[idanarye]

Okay. I can also add something like this, as a fallback:

#[derive(Clone)]
struct DynamicSkimItem(pub Arc<dyn SkimItem>);

impl DynamicSkimItem {
    pub fn new(item: impl 'static + SkimItem) -> Self {
        Self(Arc::new(item))
    }
}

impl SkimItem for DynamicSkimItem {
    ...
}

[idanarye]

Wait - does Skim clone it alot? I assume it does - otherwise it'd use Box instead of Arc. Maybe It's better to keep it as Arc<T: SkimItem>?

[kimono-koans]

Wait - does Skim clone it alot? I assume it does - otherwise it'd use Box instead of Arc. Maybe It's better to keep it as Arc<T: SkimItem>?

It needs to be an Arc for a few reasons. Yes, two_percent dedups using clone, and SkimItems must exist on many lists (there is an ItemPool and a MatchedItem Vec, etc). It also needs to be Send + Sync as ingest uses a crossbeam queue and at least two threads. Now could it be architected differently? Maybe, but that's a bigger project.

See: https://github.com/kimono-koans/two_percent/blob/vendored/src/helper/ingest.rs

[idanarye]

I'm going to have to remove this method:

two_percent/src/item.rs

Lines 73 to 75 in 9d50cad

	pub fn upgrade_infallible(&self) -> Arc<dyn SkimItem> {
	self.item.upgrade().unwrap_or(Arc::new(""))
	}

This mostly means that places that treated released data as empty strings will need to change. I don't think the behavior itself is going to change much, because this was already kind of an error...

[idanarye]

Ran into a problem. The Default for SkimOptions assigns to cmd_collector a SkimItemReader, which converts deep down converts text to DefaultSkimItem (or Box<str>) using:

two_percent/src/helper/ingest.rs

Lines 95 to 127 in 9649247

	fn send(
	line: &str,
	opts: &SendRawOrBuild,
	tx_item: &Sender<Arc<dyn SkimItem>>,
	string_intern: &mut HashMap<u64, Weak<dyn SkimItem>, BuildHasherDefault<NoHashHasher<u64>>>,
	) -> Result<(), SendError<Arc<dyn SkimItem>>> {
	let key = hash(&line.as_bytes());
	match string_intern.get(&key).and_then(|value| Weak::upgrade(value)) {
	Some(value) => tx_item.send(value),
	None => {
	let item: Arc<dyn SkimItem> = match opts {
	SendRawOrBuild::Build(opts) => {
	let item = DefaultSkimItem::new(
	line,
	opts.ansi_enabled,
	opts.trans_fields,
	opts.matching_fields,
	opts.delimiter,
	);
	Arc::new(item)
	}
	SendRawOrBuild::Raw => {
	let item: Box<str> = line.into();
	Arc::new(item)
	}
	};
	string_intern.insert_unique_unchecked(key, Arc::downgrade(&item));
	tx_item.send(item)
	}
	}
	}

This is not something I can make generic, because that will require any implementation of SkimItem to be FromStr (or have something with similar semantics) which is not rally suitable for custom skim items.

I may have to settler for restoring 'static + AsAny...

[kimono-koans]

#14