Skip to content

Commit

Permalink
[jsinterp] Avoid double key lookup for setting new key
Browse files Browse the repository at this point in the history 
In order to add a new key to both __objects and __functions dicts on jsinterp.py, it is
necessary to first verify if a key was present and if not, create the key and
assign it to a value.

However, this can be done with a single step using dict setdefault method.
  • Loading branch information
@lucasmoura @dstftw
lucasmoura authored and dstftw committed Jun 18, 2016
1 parent 5895687 commit 7c05097
Showing 1 changed file with 3 additions and 5 deletions.
8 changes: 3 additions & 5 deletions youtube_dl/jsinterp.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -131,9 +131,8 @@ def interpret_expression(self, expr, local_vars, allow_recursion):
if variable in local_vars:
obj = local_vars[variable]
else:
if variable not in self._objects:
self._objects[variable] = self.extract_object(variable)
obj = self._objects[variable]
obj = self._objects.setdefault(
variable, self.extract_object(variable))

if arg_str is None:
# Member access
Expand Down Expand Up @@ -204,8 +203,7 @@ def interpret_expression(self, expr, local_vars, allow_recursion):
argvals = tuple([
int(v) if v.isdigit() else local_vars[v]
for v in m.group('args').split(',')])
if fname not in self._functions:
self._functions[fname] = self.extract_function(fname)
self._functions.setdefault(fname, self.extract_function(fname))
return self._functions[fname](argvals)

raise ExtractorError('Unsupported JS expression %r' % expr)
Expand Down

3 comments on commit 7c05097

@ShaddyShow
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a huge regression performance wise.

After the commit above doing this:

xxx@xxx:~/youtube-dl# ./youtube-dl http://youtube.com/?v=qim0Fzozd3Qg -f 18 --no-cache-dir -v

Makes the followning ultra slow:

[youtube] {18} signature length 41.41, html5 player en_US-vfldefdPl
[youtube] {5} signature length 41.41, html5 player en_US-vfldefdPl
[youtube] {36} signature length 41.41, html5 player en_US-vfldefdPl
[youtube] {17} signature length 41.41, html5 player en_US-vfldefdPl
[youtube] {133} signature length 41.41, html5 player en_US-vfldefdPl
[youtube] {242} signature length 41.41, html5 player en_US-vfldefdPl
[youtube] {243} signature length 41.41, html5 player en_US-vfldefdPl
[youtube] {160} signature length 41.41, html5 player en_US-vfldefdPl
[youtube] {278} signature length 41.41, html5 player en_US-vfldefdPl
[youtube] {140} signature length 41.41, html5 player en_US-vfldefdPl
[youtube] {171} signature length 41.41, html5 player en_US-vfldefdPl
[youtube] {249} signature length 41.41, html5 player en_US-vfldefdPl
[youtube] {250} signature length 41.41, html5 player en_US-vfldefdPl
[youtube] {251} signature length 41.41, html5 player en_US-vfldefdPl

Reverting makes it fast again.

@phihag
Copy link
Contributor

@phihag phihag commented on 7c05097 Jun 20, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

if k not in d:
    d[k] = x

is only equivalent to

d.setdefault(k, x)

if x is idempotent. While I think that idempotence may be true here by accident, the commit fails to contain a rationale of why that should be the case.

When x is idempotent but expensive to compute, the code may slow down. Here, that seems to be the case, judging by @ShaddyShow's post.

Therefore, I have reverted this change in 1f749b6 .

@ShaddyShow
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the super fast reaction, the revert and the publishing of the new version.

All back to normal :)

Please sign in to comment.