markup: microoptimise for many short filenames in directory #1534

mappu · 2017-04-23T01:42:29Z

Move strings.ToLower() after the early-return length check. This is a safe operation in all cases and should slightly improve directory listing performance when a directory contains many thousands of files with short filenames.

lunny · 2017-04-23T03:11:51Z

LGTM

sapk · 2017-04-23T11:50:56Z

modules/markup/markup.go

+	}
+
+	name = strings.ToLower(name)
+	if len(name) == 6 {
 		return name == "readme"
 	}
 	return name[:7] == "readme."


Couldn't this be simplify by just return name == "readme" || name[:7] == "readme." and remove if len(name) == 6 bracket ?

Or just return/check name[:6] == "readme" this would also validate file like README_LANGUAGE.md ?

@mappu can you update the source code base on @sapk suggestions?

return name == "readme" || name[:7] == "readme."

cez81 · 2017-04-26T07:21:38Z

@mappu Try rebasing to latest master to pass Drone build

appleboy · 2017-04-26T08:06:47Z

modules/markup/markup.go

-	return name[:7] == "readme."
+
+	name = strings.ToLower(name)
+	return (name == "readme" || name[:7] == "readme.")


just return name == "readme" || name[:7] == "readme."

or just

func IsReadmeFile(name string) bool { return len(name) > 6 && strings.ToLower(name)[:6] == "readme" }

I never really understood why we check readme. specifically...

That what I said in my second comment, this would also validate file like README_LANGUAGE.md but maybe there is a reason ^^. I don't know in which case it is call exactly.

We only support the file name format like readme or readme.md.

@appleboy we don't really have an explicit list of files we support. :)

Yes. I think we could also support README_LANGUAGE if README is not exist.

But that's another PR's thing, this PR is a refactor.

@lunny @bkcsoft Yes. we can create another PR to discuss support list. This PR just a refactor.

sapk · 2017-04-30T14:01:18Z

@mappu Could you rebase like @cez81 suggest and make the change requested by @appleboy return (name == "readme" || name[:7] == "readme.") -> return name == "readme" || name[:7] == "readme." ?

After that you will have my L-G-T-M.

lunny · 2017-05-02T03:26:02Z

return name == "readme" || (len(name) > 6 && name[:7] == "readme.")

sapk · 2017-05-02T08:45:43Z

@lunny the goal of this PR is to do strings.ToLower(name) after checking length and not before. Your return will need to do it every time before.

lunny · 2017-05-02T09:05:08Z

@sapk but IsReadmeFile("readmf")?

sapk · 2017-05-02T11:27:54Z

@lunny I think, i didn't explain myself well enough.

What, I wanted to say is :

Check length (if > 6 return false)
toLower
return return name == "readme" || name[:7] == "readme."

That way we only do toLower if name length is good. That is the purpose of this PR.

Move strings.ToLower() after the early-return length check.

Your suggestion is good functionally but keep the same operation in a more simple way but not the micro-optimization that come with this re-ordering.

@mappu any news ?

lunny · 2017-05-02T11:40:33Z

@sapk I agree with you the three steps. I mean the last step should be return name == "readme" || (len(name) > 6 && name[:7] == "readme.") but not return name == "readme" || name[:7] == "readme." because readmf will result name[:7] panic.

sapk · 2017-05-02T11:42:15Z

@lunny good catch

bkcsoft · 2017-05-02T13:37:55Z

which was why I recommended this

return len(name) >= 6 && strings.ToLower(name)[:6] == "readme"

lunny · 2017-05-02T15:06:42Z

@bkcsoft but readmes will be rendered? and readme will not be rendered?

bkcsoft · 2017-05-02T19:43:52Z

@lunny sorry, that's len(name) >= 6. Updated 😂 And IMO we should not care what extension it is, if it begins with readme just render it :)

appleboy · 2017-05-04T04:18:33Z

agree with @lunny solution.

Move strings.ToLower() after the early-return length check. This is a safe operation in all cases and should slightly improve directory listing performance when a directory contains many thousands of files with short filenames.

mappu · 2017-05-07T04:34:59Z

Thanks for the code review.

I restructured the patch to only make a smaller, more-targeted change to the original function, and added test case for readmf.

sapk · 2017-05-09T13:46:58Z

I would consider it still a improvement even if we should talk more (in other PR) about what this func should match this is not the goal of this PR.
LGTM
👍 for the test case.

) (#2241) * Cleaning up public/ and documenting js/css libs. This commit mostly addresses #1484 by moving vendor'ed plugins into a vendor/ directory and documenting their upstream source and license in vendor/librejs.html. This also proves gitea is using only open source js/css libraries which helps toward reaching #1524. * Removing unused css file. The version of this file in use is located at: vendor/plugins/highlight/github.css * Cleaned up librejs.html and added javascript header A SafeJS function was added to templates/helper.go to allow keeping comments inside of javascript. A javascript comment was added in the header of templates/base/head.tmpl to mark all non-inline source as free. The librejs.html file was updated to meet the current librejs spec. I have now verified that the librejs plugin detects most of the scripts included in gitea and suspect the non-free detections are the result of a bug in the plugin. I believe this commit is enough to meet the C0.0 requirement of #1534. * Updating SafeJS function per lint suggestion * Added VERSIONS file, per request

lunny added this to the 1.2.0 milestone Apr 23, 2017

lunny added the type/refactoring Existing code has been cleaned up. There should be no new functionality. label Apr 23, 2017

tboerger added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Apr 23, 2017

sapk reviewed Apr 23, 2017

View reviewed changes

appleboy reviewed Apr 26, 2017

View reviewed changes

mappu force-pushed the mappu-patch-isreadmefile branch from bf395d5 to 5e8f12c Compare May 7, 2017 02:29

mappu force-pushed the mappu-patch-isreadmefile branch from 5e8f12c to ac81c8f Compare May 7, 2017 02:45

mappu added 2 commits May 7, 2017 15:59

markup: expand test cases for IsReadmeFile()

aa36c4c

mappu force-pushed the mappu-patch-isreadmefile branch from ac81c8f to aa36c4c Compare May 7, 2017 04:01

tboerger added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels May 9, 2017

lunny merged commit fd76f09 into go-gitea:master May 9, 2017

MTecknology mentioned this pull request Aug 1, 2017

Unsourced/undocumented libraries; missing license files; and other issues #1484

Closed

go-gitea locked and limited conversation to collaborators Nov 23, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

markup: microoptimise for many short filenames in directory #1534

markup: microoptimise for many short filenames in directory #1534

mappu commented Apr 23, 2017

lunny commented Apr 23, 2017

sapk Apr 23, 2017 •

edited

Loading

sapk Apr 23, 2017

appleboy Apr 26, 2017

cez81 commented Apr 26, 2017

appleboy Apr 26, 2017

bkcsoft Apr 26, 2017

sapk Apr 26, 2017 •

edited

Loading

appleboy Apr 28, 2017

bkcsoft Apr 28, 2017

lunny Apr 29, 2017

lunny Apr 29, 2017

appleboy Apr 29, 2017

sapk commented Apr 30, 2017

lunny commented May 2, 2017

sapk commented May 2, 2017

lunny commented May 2, 2017

sapk commented May 2, 2017

lunny commented May 2, 2017

sapk commented May 2, 2017

bkcsoft commented May 2, 2017 •

edited

Loading

lunny commented May 2, 2017

bkcsoft commented May 2, 2017

appleboy commented May 4, 2017

mappu commented May 7, 2017

sapk commented May 9, 2017

markup: microoptimise for many short filenames in directory #1534

markup: microoptimise for many short filenames in directory #1534

Conversation

mappu commented Apr 23, 2017

lunny commented Apr 23, 2017

sapk Apr 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cez81 commented Apr 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sapk Apr 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sapk commented Apr 30, 2017

lunny commented May 2, 2017

sapk commented May 2, 2017

lunny commented May 2, 2017

sapk commented May 2, 2017

lunny commented May 2, 2017

sapk commented May 2, 2017

bkcsoft commented May 2, 2017 • edited Loading

lunny commented May 2, 2017

bkcsoft commented May 2, 2017

appleboy commented May 4, 2017

mappu commented May 7, 2017

sapk commented May 9, 2017

sapk Apr 23, 2017 •

edited

Loading

sapk Apr 26, 2017 •

edited

Loading

bkcsoft commented May 2, 2017 •

edited

Loading