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

Optimization - avoid temporary list objects, unnecessary function call #1481

Merged
merged 1 commit into from Sep 14, 2015

Conversation

@mlyundin
Copy link
Contributor

@mlyundin mlyundin commented Sep 4, 2015

Please review.

@@ -173,7 +173,7 @@ def stop(self):
Returns a deferred that is fired when they all have ended.
"""
return defer.DeferredList([c.stop() for c in list(self.crawlers)])
return defer.DeferredList(c.stop() for c in list(self.crawlers))

This comment has been minimized.

@dangra

dangra Sep 4, 2015
Member

Old twisted versions doesn't accept a generator as DeferredList argument.

btw, why the list(self.crawlers) call?

This comment has been minimized.

@mlyundin

mlyundin Sep 4, 2015
Author Contributor

Ok. Thank you.

About crawlers I have no idei why...

This comment has been minimized.

@dangra

dangra Sep 4, 2015
Member

but if it is not iterable it will fail to build a list too

This comment has been minimized.

@mlyundin

mlyundin Sep 4, 2015
Author Contributor

I agree, That is why I gave updated my comment before)

@mlyundin mlyundin force-pushed the mlyundin:develop branch from 918c481 to 5160523 Sep 4, 2015
@codecov-io
Copy link

@codecov-io codecov-io commented Sep 4, 2015

Current coverage is 82.35%

Merging #1481 into master will not affect coverage as of 8e6f866

Powered by Codecov. Updated on successful CI builds.

@@ -4,15 +4,11 @@
import six

from scrapy.spiders import Spider
from scrapy.utils.misc import arg_to_iter
from scrapy.utils.misc import arg_to_iter as iterate_spider_output

This comment has been minimized.

@kmike

kmike Sep 4, 2015
Member

if we go this way it worths adding a comment about why is there an unused import

This comment has been minimized.

@mlyundin

mlyundin Sep 5, 2015
Author Contributor

Done.

@mlyundin mlyundin force-pushed the mlyundin:develop branch from 5160523 to 0d82811 Sep 4, 2015
@@ -975,7 +975,7 @@ class ContentDecoderAgent(object):
def __init__(self, agent, decoders):
self._agent = agent
self._decoders = dict(decoders)
self._supported = ','.join([decoder[0] for decoder in decoders])
self._supported = ','.join(decoder[0] for decoder in decoders)

This comment has been minimized.

@dangra

dangra Sep 7, 2015
Member

do not modify scrapy.xlib.client code, it is a verbatim copy of twisted sources for the specific case of supporting http1.1 in old twisted version.

We are more close to remove this code than improving it.

This comment has been minimized.

@mlyundin

mlyundin Sep 8, 2015
Author Contributor

Done.


logger = logging.getLogger(__name__)


def iterate_spider_output(result):
return arg_to_iter(result)

This comment has been minimized.

@dangra

dangra Sep 7, 2015
Member

I think the question is why do we maintain this alias and do not deprecate it instead.

This comment has been minimized.

@mlyundin

mlyundin Sep 8, 2015
Author Contributor

So What should we do?

This comment has been minimized.

@dangra

dangra Sep 8, 2015
Member

leave this change out of this PR so we can merge and discuss in another PR :)

@mlyundin mlyundin force-pushed the mlyundin:develop branch from 0d82811 to 3ebe598 Sep 8, 2015
@mlyundin mlyundin force-pushed the mlyundin:develop branch from 3ebe598 to be7821a Sep 8, 2015
@nramirezuy
Copy link
Contributor

@nramirezuy nramirezuy commented Sep 8, 2015

@mlyundin can you rebase to master?

@mlyundin
Copy link
Contributor Author

@mlyundin mlyundin commented Sep 8, 2015

I suppose I have already rebased my branch to master

$ git rebase upstream/master
Current branch develop is up to date.

@mlyundin
Copy link
Contributor Author

@mlyundin mlyundin commented Sep 14, 2015

Review hung. Is there action required from my side?

@nramirezuy
Copy link
Contributor

@nramirezuy nramirezuy commented Sep 14, 2015

@kmike @dangra are we missing tests for the code changed?

@dangra
Copy link
Member

@dangra dangra commented Sep 14, 2015

@nramirezuy I don't think so. +1 to merge

@kmike
Copy link
Member

@kmike kmike commented Sep 14, 2015

Well yes, I have two reservations for this PR. The first is that some of them are unnecessary, and they don't optimize anything: generator expressions are slower than list comprehensions, and memory savings in most places where code is changed are not important (slowdown is not important either though). It is not like anyone has 100000 spider contracts or 10000-segment spider module names.

Second is that the PR makes changes to untested parts of code (coverage reports shows this). For example, if ContractsManager would have required a list (like DeferredList did in old Twisted versions) we may not notice it.

That said, I agree with some of the cleanups; they are not optimizations, but they remove unnecessary braces in source code. So +1 to merge it if either changes to untested code are removed or new tests are added.

@@ -88,8 +88,8 @@ def _genspider(self, module, name, domain, template_name, template_file):
'module': module,
'name': name,
'domain': domain,
'classname': '%sSpider' % ''.join([s.capitalize() \
for s in module.split('_')])
'classname': '%sSpider' % ''.join(s.capitalize() \

This comment has been minimized.

@dangra

dangra Sep 14, 2015
Member

backslash is not needed

@dangra
Copy link
Member

@dangra dangra commented Sep 14, 2015

@kmike comments are fair but I think it is more clean code for same functionality when performance is not measured or taken into consideration at all.

@kmike
Copy link
Member

@kmike kmike commented Sep 14, 2015

Yeah, I also like the look of the updated code more.

@nramirezuy
Copy link
Contributor

@nramirezuy nramirezuy commented Sep 14, 2015

There are 2 lines untested; we can strip those 2 and merge the rest, create a new PR with these 2 lines?

@dangra
Copy link
Member

@dangra dangra commented Sep 14, 2015

@nramirezuy I don't think that is needed, the untested looks fine.

kmike added a commit that referenced this pull request Sep 14, 2015
cleanup
@kmike kmike merged commit 3e13740 into scrapy:master Sep 14, 2015
1 of 2 checks passed
1 of 2 checks passed
codecov/patch 75.00% of diff hit (target 100.00%)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@kmike
Copy link
Member

@kmike kmike commented Sep 14, 2015

Ok, let's just merge this :) Thanks @mlyundin for the patience!

@dangra
Copy link
Member

@dangra dangra commented Sep 14, 2015

enough 🚲 🎪 !

@nramirezuy
Copy link
Contributor

@nramirezuy nramirezuy commented Sep 14, 2015

🐻

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants