Inline file_uri_scheme() in file_stream_wrapper_uri_normalize() and other file.inc functions

Features need to go against d8

Currently, 30 separate functions call file_uri_scheme(). For consistency's sake, I suspect we would need to inline *all* instances of this field rather than just one. I'm not sure that makes sense.

oops, triggered testbot on 8.x by accident. Meant to retest 7.x

then set issue to 8.x after test

Patch still passes 7.x dev, back to 8.0.x dev

I'm feeling lucky.

function file_uri_scheme is deprecated in 8.x

Someone made a wrapper that calls \Drupal::service('file_system')->uriScheme instead

here's a patch to replace file_uri_scheme

Since there is no API change here, it looks like just clean-up (if it passes testbot), I'm adding to the RC target triage.

ah ok, just looking over this, it looks like the inline file_uri_scheme is faster than the actual function call despite the fact that the code inside the function is exactly the same.

Looks like the reported performance gains comparably due to one less copy of the parameter going into the function. The gains are made by avoiding the parameter copy. This makes sense when having to process hundreds, or more likely thousands or tens of thousands of file uri variables typical of what we see in the files folder.

However in 8.x the file_url_schema function is supposed to be deprecated but has not yet been removed.

This doesn't look like a feature, it looks like clean-up to me. May need a title and issue summary refactoring for what you are doing now.

Ok sounds good *EDIT*, maybe just create a new issue for the cleanup and set it as a related issue to this one, I was more focused on the performance issue for D7 just doing D8 work to push the issue along.
*EDIT*

Thanks for tagging this for rc target triage! If you want committers to consider it for inclusion in RC, add a statement to the issue summary of why we need to make this change during RC, including what happens if we do not make the change and what any disruptions from it are. You can add a section <h3>Why this should be an RC target</h3> to the summary.

Thanks @joseph.olstad!

This seems like a good rc target to me based on that summary. I pinged other committers for a second opinion.

In my opinion, removing calls to @deprecated wrappers qualifies as a non-disruptive change to comply with coding standards, so tagging "rc eligible" per https://www.drupal.org/core/d8-allowed-changes#rc.

#2470679: [meta] Identify necessary performance optimizations for common profiling scenarios lists "Over ~100ms or more savings with cold caches and would have to be deferred to a minor version" as a condition that could warrant an issue being Critical. This one wouldn't need to be deferred to a minor version, so not raising this to Critical, but I do think that kind of performance savings justifies raising it to Major.

In my opinion, removing calls to @deprecated wrappers qualifies as a non-disruptive change to comply with coding standards, so tagging "rc eligible"

Removing deprecated code is not really a coding standards change per se (IMO), because it's actually changing the behavior of the code (even if it's just removing one function call from the stack). So swapping this to RC target (for the performance improvements).

Note that there are 39 uses/references to the procedural function still remaining after the patch which can be done in 8.1.x unless there are similar performance improvements. The patch removes 5.

Committed and pushed to 8.0.x. Thanks for the fix and the profiling!

Hi as this is already committed to D8, assigning back to D7 for re-test.

Triggering test bot for D7. The D8 patch was already committed.

Same patch as #1 @mikeytown , just re-uploading it as the testbot was updated after the first patch was uploaded so probably needs to be re-triggered this way.

Testbot success, set to rtbc and retest will be triggered again

This seems to be a nice performance improvement -- maybe let's clarify the comment a bit?

Assigning to Fabianx for final review and commit

Sitting at Dublin next to Fabian -- he reviewed and this is fine. Let's just clarify the comment to: "Inline file_uri_scheme() function call for performance reasons"

I'm doing my first patch in Drupalcon Dublin with stefan.r

Seems the the easy patch asked by @stefan.r on #37 is done correctly.

Congrats @LauraRocks for your first patch ;)

Adding credit

RTBC!

Committed and pushed LIVE at DrupalCon Double-N! [sic] WOOOOOOOO!!!