Much more efficient memory usage and smarter dynamic dispatch usage,... #210

Vrtgs · 2024-04-13T04:59:40Z

…and a bunch of kinda nit-picky changes to docs (including typo fixes)

I ran the tests this time, I learned, but this time I only ran chrome, because I just could not get the gekodriver to work properly

…and a bunch of kinda nit-picky changes to docs (including typo fixes)

Vrtgs · 2024-04-13T05:00:27Z

oh yeah msvc is probably 1.75 if that matters too much I'm gonna have to fix that

stevepryde

Thanks for the PR.

I'm curious to know why you felt memory usage was an issue, especially for a web browser automation crate. What is your use-case? Neither performance nor memory usage are usually critical for this kind of software. That said, I think the ergonomics from a user perspective is mostly unaffected by the changes and in some cases ergonomics is slightly improved (such as element filtering predicates etc) so I'm happy to go ahead with it.

Since we're nit-picking, I've flagged the cases where you've changed the Australian spelling of certain words to American. I'm assuming it wasn't intentional. Normally I wouldn't care too much about the spelling of new contributions, but changing existing documentation to American spelling is a bit much😛.

stevepryde · 2024-04-13T07:42:53Z

thirtyfour/src/session/mod.rs

+    use std::rc::Rc;
+    use std::sync::Arc;
+
+    pub trait IntoTransfer {


Why IntoTransfer? What about IntoArcStr or something more specific?

Transfer, because were gonna transfer it to the other management task

IntoArcStr would be confusing because why not use Into<Arc> which I didn't use because many of the types added to the impl don't implement Into<Arc>

if you have name suggestions I'm up for it I tried to come up with a good name for a good 10 minutes

docs/src/features/components.md

thirtyfour-macros/src/component.rs

thirtyfour/src/common/capabilities/desiredcapabilities.rs

thirtyfour/src/components/select.rs

thirtyfour/src/extensions/query/conditions.rs

thirtyfour/src/extensions/query/element_waiter.rs

thirtyfour/src/extensions/query/mod.rs

thirtyfour/tests/components.rs

stevepryde · 2024-04-13T08:29:24Z

oh yeah msvc is probably 1.75 if that matters too much I'm gonna have to fix that

Personally I don't mind, but some users might not like it.
How much work it is to support versions back 12 months or something like that? Somewhere around 1.69 or 1.70. That's still arbitrary but might be nicer for some users if they're stuck on an older version for whatever reason.

stevepryde · 2024-04-13T08:38:24Z

…and a bunch of kinda nit-picky changes to docs (including typo fixes)

I ran the tests this time, I learned, but this time I only ran chrome, because I just could not get the gekodriver to work properly

What OS? Wondering if there's any documentation updates we can add to fix this.
https://stevepryde.github.io/thirtyfour/contributing/testing.html

I am on Windows WSL but I've tried it on Linux previously. On Ubuntu you can just install the firefox snap and then run geckodriver from a terminal. Then in another terminal/tab you can run the tests using THIRTYFOUR_BROWSER=firefox cargo test.

Vrtgs · 2024-04-13T14:40:44Z

oh yeah msvc is probably 1.75 if that matters too much I'm gonna have to fix that

Personally I don't mind, but some users might not like it. How much work it is to support versions back 12 months or something like that? Somewhere around 1.69 or 1.70. That's still arbitrary but might be nicer for some users if they're stuck on an older version for whatever reason.

I think we can do that by just adding #[async_trait] to
thirtyfour::components::wrapper::resolver::sealed::Resolve

Vrtgs · 2024-04-13T14:42:33Z

…and a bunch of kinda nit-picky changes to docs (including typo fixes)
I ran the tests this time, I learned, but this time I only ran chrome, because I just could not get the gekodriver to work properly

What OS? Wondering if there's any documentation updates we can add to fix this. stevepryde.github.io/thirtyfour/contributing/testing.html

I am on Windows WSL but I've tried it on Linux previously. On Ubuntu you can just install the firefox snap and then run geckodriver from a terminal. Then in another terminal/tab you can run the tests using THIRTYFOUR_BROWSER=firefox cargo test.

running geckodriver on the terminal displays a "Machine type Mismatch"

codecov-commenter · 2024-04-13T15:00:58Z

Codecov Report

Attention: Patch coverage is 37.22826% with 231 lines in your changes are missing coverage. Please review.

Project coverage is 41.31%. Comparing base (f2b9a13) to head (47a11ef).

❗ Current head 47a11ef differs from pull request most recent head 45cbafc. Consider uploading reports for the commit 45cbafc to get more accurate results

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

Files	Coverage Δ
thirtyfour/src/action_chain.rs	`61.25% <ø> (ø)`
thirtyfour/src/alert.rs	`73.33% <ø> (ø)`
thirtyfour/src/common/capabilities/firefox.rs	`0.00% <ø> (ø)`
thirtyfour/src/common/command.rs	`78.21% <100.00%> (ø)`
thirtyfour/src/common/requestdata.rs	`47.61% <100.00%> (ø)`
thirtyfour/src/extensions/query/poller.rs	`100.00% <ø> (ø)`
thirtyfour/src/web_element.rs	`87.50% <100.00%> (ø)`
thirtyfour/tests/components.rs	`100.00% <ø> (ø)`
thirtyfour-macros/src/lib.rs	`0.00% <0.00%> (ø)`
thirtyfour/src/common/capabilities/chromium.rs	`10.12% <0.00%> (-1.37%)`	⬇️
... and 16 more

Vrtgs · 2024-04-13T17:00:42Z

I'm curious to know why you felt memory usage was an issue, especially for a web browser automation crate. What is your use-case? Neither performance nor memory usage are usually critical for this kind of software

a bit of a dumb reason honestly what sparked my initial snooping in the code was I got mad that I had to wrap everything in Box::new and Box::pin for the resolver, and looked at the code

and honestly the thing that ticked me off the most to go read the entire codebase and do these changes was because I saw you storing Arc<ElementQueryFn> but ElementQueryFn was Box<dyn ...>, yes again I realize its a bit of a waste to care too much, I ended up taking it as a challenge for the most part, not that it was that bad for performance

… dependencie

…HttpClient

stevepryde · 2024-04-13T23:47:46Z

I'm curious to know why you felt memory usage was an issue, especially for a web browser automation crate. What is your use-case? Neither performance nor memory usage are usually critical for this kind of software

a bit of a dumb reason honestly what sparked my initial snooping in the code was I got mad that I had to wrap everything in Box::new and Box::pin for the resolver, and looked at the code

and honestly the thing that ticked me off the most to go read the entire codebase and do these changes was because I saw you storing Arc but ElementQueryFn was Box<dyn ...>, yes again I realize its a bit of a waste to care too much, I ended up taking it as a challenge for the most part, not that it was that bad for performance

Yep I like the changes you made in that area. The predicate code feels a bit nicer now.

stevepryde · 2024-04-13T23:48:26Z

…and a bunch of kinda nit-picky changes to docs (including typo fixes)
I ran the tests this time, I learned, but this time I only ran chrome, because I just could not get the gekodriver to work properly

What OS? Wondering if there's any documentation updates we can add to fix this. stevepryde.github.io/thirtyfour/contributing/testing.html
I am on Windows WSL but I've tried it on Linux previously. On Ubuntu you can just install the firefox snap and then run geckodriver from a terminal. Then in another terminal/tab you can run the tests using THIRTYFOUR_BROWSER=firefox cargo test.

running geckodriver on the terminal displays a "Machine type Mismatch"

That's odd. What OS? What arch? Are you downloading the correct version of geckodriver?

stevepryde · 2024-04-13T23:49:00Z

oh yeah msvc is probably 1.75 if that matters too much I'm gonna have to fix that

Personally I don't mind, but some users might not like it. How much work it is to support versions back 12 months or something like that? Somewhere around 1.69 or 1.70. That's still arbitrary but might be nicer for some users if they're stuck on an older version for whatever reason.

I think we can do that by just adding #[async_trait] to thirtyfour::components::wrapper::resolver::sealed::Resolve

I think this is worth doing

stevepryde · 2024-04-13T23:51:00Z

Once you're done making changes, please provide a summary of the changes and the reasons for them. I feel that these later changes probably should have been a separate PR. Let's not try to change the entire codebase in one commit 😅

stevepryde · 2024-04-14T00:03:29Z

thirtyfour/src/components/select.rs

+impl Display for Escaped<'_> {
+    fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
+        for (i, substring) in self.0.split('\"').enumerate() {
+            if i != 0 {
+                f.write_str(", '\"', ")?
+            }
+            write!(f, "\"{}\"", substring)?;
+        }
+        if self.0.ends_with('\"') {
+            f.write_str(", '\"'")?;
+        }
+        Ok(())
+    }
+}


This could have just been a function 🤔 ?

I don't think so, because I put escaped into a call of format!() and I don't think there is a way to access the formatter directly

I kind of use the same patter as any of the std::str::Escape*

Vrtgs · 2024-04-14T03:40:34Z

Once you're done making changes, please provide a summary of the changes and the reasons for them. I feel that these later changes probably should have been a separate PR. Let's not try to change the entire codebase in one commit 😅

hmm yeah, I should

Vrtgs · 2024-04-14T03:42:52Z

…and a bunch of kinda nit-picky changes to docs (including typo fixes)
I ran the tests this time, I learned, but this time I only ran chrome, because I just could not get the gekodriver to work properly

What OS? Wondering if there's any documentation updates we can add to fix this. stevepryde.github.io/thirtyfour/contributing/testing.html
I am on Windows WSL but I've tried it on Linux previously. On Ubuntu you can just install the firefox snap and then run geckodriver from a terminal. Then in another terminal/tab you can run the tests using THIRTYFOUR_BROWSER=firefox cargo test.

running geckodriver on the terminal displays a "Machine type Mismatch"

That's odd. What OS? What arch? Are you downloading the correct version of geckodriver?

windows x86_64, I downloaded geckodriver-v0.33.0-win64.zip

stevepryde · 2024-04-14T04:31:56Z

I appreciate the effort you've put in, but dumping a huge code change like this as a PR makes it really difficult to sift through and understand the intent and the implications.

I like the improvements to the predicate functions but a lot of other changes seem to increase the complexity of the codebase without offering a tangible real-world benefit either to the developer or to the runtime outcome. I'd even suggest there's a fair chunk that largely comes down to personal preference. This is the first I've seen anyone recommend Arc<str> instead of String, for example. I can see why you'd care about this for a performance critical application, but this isn't that. Instead many users of the library will probably find it confusing.

I think there's an argument for keeping things familiar and not increasing the complexity unnecessarily.

Vrtgs · 2024-04-14T04:38:24Z

I appreciate the effort you've put in, but dumping a huge code change like this as a PR makes it really difficult to sift through and understand the intent and the implications.

I like the improvements to the predicate functions but a lot of other changes seem to increase the complexity of the codebase without offering a tangible real-world benefit either to the developer or to the runtime outcome. I'd even suggest there's a fair chunk that largely comes down to personal preference. This is the first I've seen anyone recommend Arc<str> instead of String, for example. I can see why you'd care about this for a performance critical application, but this isn't that. Instead many users of the library will probably find it confusing.

I think there's an argument for keeping things familiar and not increasing the complexity unnecessarily.

hmm yea I see your point, and I think I really should make a list of all the changes and detail what how and why I did them

as for Arc<str> I think should be used here because we just want an immutable view to a string slice, and as can be see from the code since we clone the string slice a lot I think it would be useful to use an Arc<str> instead of a String, and I mean most of the changes that came with that were not at the site where the string was used, just where it was created and given a type

stevepryde · 2024-04-14T04:47:03Z

I appreciate the effort you've put in, but dumping a huge code change like this as a PR makes it really difficult to sift through and understand the intent and the implications.
I like the improvements to the predicate functions but a lot of other changes seem to increase the complexity of the codebase without offering a tangible real-world benefit either to the developer or to the runtime outcome. I'd even suggest there's a fair chunk that largely comes down to personal preference. This is the first I've seen anyone recommend Arc<str> instead of String, for example. I can see why you'd care about this for a performance critical application, but this isn't that. Instead many users of the library will probably find it confusing.
I think there's an argument for keeping things familiar and not increasing the complexity unnecessarily.

hmm yea I see your point, and I think I really should make a list of all the changes and detail what how and why I did them

as for Arc<str> I think should be used here because we just want an immutable view to a string slice, and as can be see from the code since we clone the string slice a lot I think it would be useful to use an Arc<str> instead of a String, and I mean most of the changes that came with that were not at the site where the string was used, just where it was created and given a type

I understand the reason for the Arc<str>. It's not a bad change. There is some benefit like you said. And I think you did it in a way that doesn't affect how the library is used. It seems to be an internal change only for the most part. So that's fine.

Don't worry about splitting it or anything. Let's just put a freeze on the feature changes and I'll have another look through the code and we can go from there.

If you can summarise the changes with a bullet point each that would be helpful too.

Vrtgs · 2024-04-14T05:28:21Z

I appreciate the effort you've put in, but dumping a huge code change like this as a PR makes it really difficult to sift through and understand the intent and the implications.
I like the improvements to the predicate functions but a lot of other changes seem to increase the complexity of the codebase without offering a tangible real-world benefit either to the developer or to the runtime outcome. I'd even suggest there's a fair chunk that largely comes down to personal preference. This is the first I've seen anyone recommend Arc<str> instead of String, for example. I can see why you'd care about this for a performance critical application, but this isn't that. Instead many users of the library will probably find it confusing.
I think there's an argument for keeping things familiar and not increasing the complexity unnecessarily.

hmm yea I see your point, and I think I really should make a list of all the changes and detail what how and why I did them
as for Arc<str> I think should be used here because we just want an immutable view to a string slice, and as can be see from the code since we clone the string slice a lot I think it would be useful to use an Arc<str> instead of a String, and I mean most of the changes that came with that were not at the site where the string was used, just where it was created and given a type

I understand the reason for the Arc<str>. It's not a bad change. There is some benefit like you said. And I think you did it in a way that doesn't affect how the library is used. It seems to be an internal change only for the most part. So that's fine.

Don't worry about splitting it or anything. Let's just put a freeze on the feature changes and I'll have another look through the code and we can go from there.

If you can summarise the changes with a bullet point each that would be helpful too.

here since I'm done with the first part for the first list of major changes you can read through while I write part 2

List of changes:

Some admittedly nitty spelling changes

lets start with thirtyfour-macros, the changes I made were:

as recommended by the standard library I updated the paths to use absolute paths instead of relative paths
updated the predicate to how predicates are written now

as for thirtyfour it self:

in common/capabilities
- I pulled remove_arg and args and has_arg from their specific browsers capabilities, and moved it to BrowserCapabilitiesHelper, because they are the exact same function
- changed has_arg slightly, to avoid the one extra copy of the argument made to pass into contains, and instead used .iter().any() and checked for equality directly
in common/command:
- I changed the Strings to Arc<str> to make the enclosing structs cheaply cloneable, and Arc<str> isn't any different from a normal String, so it shouldn't matter that much from an ergonomic standpoint, as you can use it in basically much the same way
- removed unnecessary prefixes for serde_json::Value
in common/types:
- much the same changes as before for Arc<str>
- improvements on predicate functions
in /components/wrapper/resolver:
- improvements to the way caching is implemented,
  the old system, there could be more than one access to resolve all of them see that there is no cached result and then both of them, will try to resolve it having many requests to the browser to resolve the element and then all of them start replacing the cached value (which also requires mutable access and requiring the use of a mutex),
  the new system I use tokio's async OnceCell, this makes sure that the query runs only once per time caching, and if it was already initialized there is a fast path that returns the already initialized value, I store this behind an ArcSwap which basically is an Arc that I can also atomically change the value it is pointing to, which allows the resolution of components to be nearly fully lock free
- use the new style of predicates
- reduce code duplication, by adding a trait to the things that can be resolved, and adding the proper validate and resolve methods onto those types, instead of duplicating the code again and again
- made the resolution of lists of Elements concurrent, by making them check the status of many elements concurrently
in extensions/query/conditions:
- changed to the new style of predicate
- removed some code duplication by introducing a macro
in extensions/query/element_query and extensions/query/element_waiter:
- changed to the new style of predicate
- much the same changes as before for Arc<str>

other changes have either been discussed or are very minor

stevepryde

Thanks for the writeup and for the awesome work. It's much appreciated!

Much more efficient memory usage and smarted dynamic dispatch usage, …

cfe34c0

…and a bunch of kinda nit-picky changes to docs (including typo fixes)

stevepryde requested changes Apr 13, 2024

View reviewed changes

Vrtgs closed this Apr 13, 2024

Vrtgs reopened this Apr 13, 2024

Vrtgs and others added 3 commits April 13, 2024 17:44

Auzie english

eaa391f

Australian spelling

26dc35f

Australian spelling

dc5fb5a

Australian spelling

9acae1b

Some more nit picking, and also using the proper datatypes

9925e99

Vrtgs requested a review from stevepryde April 13, 2024 21:36

Vrtgs added 6 commits April 14, 2024 02:03

remade a new trait for reqwest::IntoUrl to prevent from using it as a…

47a11ef

… dependencie

ran cargo fmt, and clippy

b83e2ba

fixed cargo doc warning

45cbafc

actually use IntoUrl

6f6ebc9

exposed a new_with_config_and_client method, fixed lifetime on Null…

8b78f87

…HttpClient

Removed needless HttpClient + Send + Sync bounds

db880e6

stevepryde reviewed Apr 14, 2024

View reviewed changes

removed async trait, and updated msrv to 1.70

e495b0a

pulled has_arg out of its specific browsers capabilities

768e596

stevepryde approved these changes Apr 15, 2024

View reviewed changes

stevepryde merged commit 928b9cd into Vrtgs:main Apr 15, 2024
11 of 14 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Much more efficient memory usage and smarter dynamic dispatch usage,... #210

Much more efficient memory usage and smarter dynamic dispatch usage,... #210

Vrtgs commented Apr 13, 2024 •

edited

Loading

Vrtgs commented Apr 13, 2024

stevepryde left a comment

stevepryde Apr 13, 2024

Vrtgs Apr 13, 2024

Vrtgs Apr 13, 2024 •

edited

Loading

stevepryde commented Apr 13, 2024

stevepryde commented Apr 13, 2024

Vrtgs commented Apr 13, 2024 •

edited

Loading

Vrtgs commented Apr 13, 2024

codecov-commenter commented Apr 13, 2024 •

edited

Loading

Vrtgs commented Apr 13, 2024

stevepryde commented Apr 13, 2024

stevepryde commented Apr 13, 2024

stevepryde commented Apr 13, 2024

stevepryde commented Apr 13, 2024

stevepryde Apr 14, 2024

Vrtgs Apr 14, 2024

Vrtgs commented Apr 14, 2024

Vrtgs commented Apr 14, 2024

stevepryde commented Apr 14, 2024

Vrtgs commented Apr 14, 2024

stevepryde commented Apr 14, 2024

Vrtgs commented Apr 14, 2024 •

edited

Loading

stevepryde left a comment

Much more efficient memory usage and smarter dynamic dispatch usage,... #210

Much more efficient memory usage and smarter dynamic dispatch usage,... #210

Conversation

Vrtgs commented Apr 13, 2024 • edited Loading

Vrtgs commented Apr 13, 2024

stevepryde left a comment

Choose a reason for hiding this comment

stevepryde Apr 13, 2024

Choose a reason for hiding this comment

Vrtgs Apr 13, 2024

Choose a reason for hiding this comment

Vrtgs Apr 13, 2024 • edited Loading

Choose a reason for hiding this comment

stevepryde commented Apr 13, 2024

stevepryde commented Apr 13, 2024

Vrtgs commented Apr 13, 2024 • edited Loading

Vrtgs commented Apr 13, 2024

codecov-commenter commented Apr 13, 2024 • edited Loading

Codecov Report

Vrtgs commented Apr 13, 2024

stevepryde commented Apr 13, 2024

stevepryde commented Apr 13, 2024

stevepryde commented Apr 13, 2024

stevepryde commented Apr 13, 2024

stevepryde Apr 14, 2024

Choose a reason for hiding this comment

Vrtgs Apr 14, 2024

Choose a reason for hiding this comment

Vrtgs commented Apr 14, 2024

Vrtgs commented Apr 14, 2024

stevepryde commented Apr 14, 2024

Vrtgs commented Apr 14, 2024

stevepryde commented Apr 14, 2024

Vrtgs commented Apr 14, 2024 • edited Loading

stevepryde left a comment

Choose a reason for hiding this comment

Vrtgs commented Apr 13, 2024 •

edited

Loading

Vrtgs Apr 13, 2024 •

edited

Loading

Vrtgs commented Apr 13, 2024 •

edited

Loading

codecov-commenter commented Apr 13, 2024 •

edited

Loading

Vrtgs commented Apr 14, 2024 •

edited

Loading