Bug 69589

Summary: [chromium] Let rule_binding use os.execvp() instead of subprocess.call() to spawn fewer processes.
Product: WebKit Reporter: Nico Weber <thakis>
Component: New BugsAssignee: Nico Weber <thakis>
Status: RESOLVED FIXED    
Severity: Normal CC: eric, loislo, podivilov, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 69626    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Nico Weber 2011-10-06 18:22:02 PDT 
[chromium] Let rule_binding use os.execvp() instead of subprocess.call() to spawn fewer processes.
Comment 1 Nico Weber 2011-10-06 18:25:07 PDT 
Created attachment 110073 [details]
Patch
Comment 2 WebKit Review Bot 2011-10-06 19:42:38 PDT 
Comment on attachment 110073 [details]
Patch

Clearing flags on attachment: 110073

Committed r96892: <http://trac.webkit.org/changeset/96892>
Comment 3 WebKit Review Bot 2011-10-06 19:42:42 PDT 
All reviewed patches have been landed.  Closing bug.
Comment 4 Pavel Podivilov 2011-10-07 08:57:56 PDT 
Broke chromium win build.

rule_binding.py passes *.idl file as last argument to generate-bindings.pl, however generate-bindings.pl expects *.idl file to be first argument. This all look very mysterious, and I have no idea how did it work before, but it stopped to work with subprocess on windows.
Comment 5 Pavel Podivilov 2011-10-07 08:59:15 PDT 
s/with subprocess/without subprocess/
Comment 6 Nico Weber 2011-10-07 09:20:07 PDT 
Do you have error logs anywhere?
Comment 7 Pavel Podivilov 2011-10-07 10:23:06 PDT 
http://build.chromium.org/p/chromium.webkit/builders/Webkit%20Win%20%28dbg%29%281%29/builds/7136/steps/compile/logs/stdio

Can't call method "fileName" without a package or object reference at ../bindings/scripts/IDLParser.pm line 102.
Comment 8 Pavel Podivilov 2011-10-07 10:40:02 PDT 
Not sure if it is related, but you call os.execvp('perl', command) where command is ['perl', '-w', ...].
Comment 9 Nico Weber 2011-10-07 10:42:04 PDT 
That's how exec works, argv[0] is the command name. I'll try to find a windows box and debug, thanks
Comment 10 Ilya Tikhonovsky 2011-10-07 11:37:54 PDT 
(In reply to comment #6)
> Do you have error logs anywhere?

good version of rule_binding.py (subprocess.call) is passing 36 arguments
bad version (os.execvp) is passing 111 arguments 
the difference is in the list of defines. execvp is splitting single long list of defines into separate arguments.

-----------
1>  --defines
1>  ENABLE_3D_PLUGIN=1 ENABLE_BLOB=1 ENABLE_BLOB_SLICE=1 ENABLE_CHANNEL_MESSAGING=1 ENABLE_CLIENT_BASED_GEOLOCATION=1 ENABLE_DASHBOARD_SUPPORT=0 ENABLE_DATA_TRANSFER_ITEMS=1 ENABLE_DETAILS=1 ENABLE_DEVICE_ORIENTATION=1 ENABLE_DIRECTORY_UPLOAD=1 ENABLE_DOM_STORAGE=1 ENABLE_DOWNLOAD_ATTRIBUTE=1 ENABLE_FILE_SYSTEM=1 ENABLE_FILTERS=1 ENABLE_FULLSCREEN_API=1 ENABLE_GAMEPAD=1 ENABLE_GEOLOCATION=1 ENABLE_GESTURE_EVENTS=1 ENABLE_GESTURE_RECOGNIZER=1 ENABLE_ICONDATABASE=0 ENABLE_INDEXED_DATABASE=1 ENABLE_INPUT_SPEECH=1 ENABLE_INPUT_TYPE_COLOR=0 ENABLE_INPUT_TYPE_DATE=0 ENABLE_INPUT_TYPE_DATETIME=0 ENABLE_INPUT_TYPE_DATETIMELOCAL=0 ENABLE_INPUT_TYPE_MONTH=0 ENABLE_INPUT_TYPE_TIME=0 ENABLE_INPUT_TYPE_WEEK=0 ENABLE_JAVASCRIPT_DEBUGGER=1 ENABLE_JAVASCRIPT_I18N_API=1 ENABLE_LINK_PREFETCH=1 ENABLE_MEDIA_STATISTICS=1 ENABLE_MEDIA_STREAM=1 ENABLE_METER_TAG=1 ENABLE_MHTML=1 ENABLE_MUTATION_OBSERVERS=0 ENABLE_NOTIFICATIONS=1 ENABLE_ORIENTATION_EVENTS=0 ENABLE_PAGE_VISIBILITY_API=1 ENABLE_PROGRESS_TAG=1 ENABLE_QUOTA=1 ENABLE_REQUEST_ANIMATION_FRAME=1 ENABLE_RUBY=1 ENABLE_SANDBOX=1 ENABLE_SHARED_WORKERS=1 ENABLE_SKIA_GPU=0 ENABLE_SMOOTH_SCROLLING=1 ENABLE_SQL_DATABASE=1 ENABLE_SVG=1 ENABLE_SVG_FONTS=1 ENABLE_TOUCH_EVENTS=1 ENABLE_TOUCH_ICON_LOADING=0 ENABLE_V8_SCRIPT_DEBUG_SERVER=1 ENABLE_VIDEO=1 ENABLE_VIDEO_TRACK=1 ENABLE_WEBGL=1 ENABLE_WEB_SOCKETS=1 ENABLE_WEB_TIMING=1 ENABLE_WORKERS=1 ENABLE_XHR_RESPONSE_BLOB=1 ENABLE_XPATH=1 ENABLE_XSLT=1 WTF_USE_LEVELDB=1 WTF_USE_BUILTIN_UTF8_CODEC=1 WTF_USE_OPENTYPE_SANITIZER=1 WTF_USE_WEBP=1 WTF_USE_WEBKIT_IMAGE_DECODERS=1 ENABLE_WEB_AUDIO=1 WTF_USE_ACCELERATED_COMPOSITING=1 ENABLE_3D_RENDERING=1 ENABLE_ACCELERATED_2D_CANVAS=1 WTF_USE_WEBAUDIO_FFMPEG=1 ENABLE_REGISTER_PROTOCOL_HANDLER=1 LANGUAGE_JAVASCRIPT V8_BINDING
1>  --generator
1>  V8
1>  --inclus
1>  --include
1>  ../dom
1>  --include
1>  ../fileapi
1>  --include
1>  ../html
1>  --include
1>  ../notifications
1>  --include
1>  ../page
1>  --include
1>  ../plugins
1>  --include
1>  ../storage
1>  --include
1>  ../svg
1>  --include
1>  ../testing
1>  --include
1>  ../webaudio
1>  --include
1>  ../websockets
1>  --include
1>  ../workers
1>  --include
1>  ../xml
1>  --outputHeadersDir
1>  E:/src/chromium/src/build/Release//obj/global_intermediate/webkit/bindings
1>  --outputDir
1>  E:/src/chromium/src/build/Release//obj/global_intermediate/webcore/bindings
1>  ..\html\HTMLTitleElement.idl
----------
Comment 11 Ilya Tikhonovsky 2011-10-07 12:02:49 PDT 
the bad version

before #ARGS=111
-------------
1>  --defines
1>  ENABLE_3D_PLUGIN=1
1>  ENABLE_BLOB=1
1>  ENABLE_BLOB_SLICE=1
1>  ENABLE_CHANNEL_MESSAGING=1
1>  ENABLE_CLIENT_BASED_GEOLOCATION=1
1>  ENABLE_DASHBOARD_SUPPORT=0
1>  ENABLE_DATA_TRANSFER_ITEMS=1
1>  ENABLE_DETAILS=1
1>  ENABLE_DEVICE_ORIENTATION=1
1>  ENABLE_DIRECTORY_UPLOAD=1
1>  ENABLE_DOM_STORAGE=1
1>  ENABLE_DOWNLOAD_ATTRIBUTE=1
1>  ENABLE_FILE_SYSTEM=1
1>  ENABLE_FILTERS=1
1>  ENABLE_FULLSCREEN_API=1
1>  ENABLE_GAMEPAD=1
1>  ENABLE_GEOLOCATION=1
1>  ENABLE_GESTURE_EVENTS=1
1>  ENABLE_GESTURE_RECOGNIZER=1
1>  ENABLE_ICONDATABASE=0
1>  ENABLE_INDEXED_DATABASE=1
1>  ENABLE_INPUT_SPEECH=1
1>  ENABLE_INPUT_TYPE_COLOR=0
1>  ENABLE_INPUT_TYPE_DATE=0
1>  ENABLE_INPUT_TYPE_DATETIME=0
1>  ENABLE_INPUT_TYPE_DATETIMELOCAL=0
1>  ENABLE_INPUT_TYPE_MONTH=0
1>  ENABLE_INPUT_TYPE_TIME=0
1>  ENABLE_INPUT_TYPE_WEEK=0
1>  ENABLE_JAVASCRIPT_DEBUGGER=1
1>  ENABLE_JAVASCRIPT_I18N_API=1
1>  ENABLE_LINK_PREFETCH=1
1>  ENABLE_MEDIA_STATISTICS=1
1>  ENABLE_MEDIA_STREAM=1
1>  ENABLE_METER_TAG=1
1>  ENABLE_MHTML=1
1>  ENABLE_MUTATION_OBSERVERS=0
1>  ENABLE_NOTIFICATIONS=1
1>  ENABLE_ORIENTATION_EVENTS=0
1>  ENABLE_PAGE_VISIBILITY_API=1
1>  ENABLE_PROGRESS_TAG=1
1>  ENABLE_QUOTA=1
1>  ENABLE_REQUEST_ANIMATION_FRAME=1
1>  ENABLE_RUBY=1
1>  ENABLE_SANDBOX=1
1>  ENABLE_SHARED_WORKERS=1
1>  ENABLE_SKIA_GPU=0
1>  ENABLE_SMOOTH_SCROLLING=1
1>  ENABLE_SQL_DATABASE=1
1>  ENABLE_SVG=1
1>  ENABLE_SVG_FONTS=1
1>  ENABLE_TOUCH_EVENTS=1
1>  ENABLE_TOUCH_ICON_LOADING=0
1>  ENABLE_V8_SCRIPT_DEBUG_SERVER=1
1>  ENABLE_VIDEO=1
1>  ENABLE_VIDEO_TRACK=1
1>  ENABLE_WEBGL=1
1>  ENABLE_WEB_SOCKETS=1
1>  ENABLE_WEB_TIMING=1
1>  ENABLE_WORKERS=1
1>  ENABLE_XHR_RESPONSE_BLOB=1
1>  ENABLE_XPATH=1
1>  ENABLE_XSLT=1
1>  WTF_USE_LEVELDB=1
1>  WTF_USE_BUILTIN_UTF8_CODEC=1
1>  WTF_USE_OPENTYPE_SANITIZER=1
1>  WTF_USE_WEBP=1
1>  WTF_USE_WEBKIT_IMAGE_DECODERS=1
1>  ENABLE_WEB_AUDIO=1
1>  WTF_USE_ACCELERATED_COMPOSITING=1
1>  ENABLE_3D_RENDERING=1
1>  ENABLE_ACCELERATED_2D_CANVAS=1
1>  WTF_USE_WEBAUDIO_FFMPEG=1
1>  ENABLE_REGISTER_PROTOCOL_HANDLER=1
1>  LANGUAGE_JAVASCRIPT
1>  V8_BINDING
1>  --generator
1>  V8
1>  --include
1>  ../css
1>  --include
1>  ../dom
1>  --include
1>  ../fileapi
1>  --include
1>  ../html
1>  --include
1>  ../notifications
1>  --include
1>  ../page
1>  --include
1>  ../plugins
1>  --include
1>  ../storage
1>  --include
1>  ../svg
1>  --include
1>  ../testing
1>  --include
1>  ../webaudio
1>  --include
1>  ../websockets
1>  --include
1>  ../workers
1>  --include
1>  ../xml
1>  --outputHeadersDir
1>  E:/src/chromium/src/build/Release//obj/global_intermediate/webkit/bindings
1>  --outputDir
1>  E:/src/chromium/src/build/Release//obj/global_intermediate/webcore/bindings
1> ..\css\RGBColor.idl


after calling GetOptions almost all the arguments still in the list
------------------------
1>  after ENABLE_BLOB=1 ENABLE_BLOB_SLICE=1 ENABLE_CHANNEL_MESSAGING=1 ENABLE_CLIENT_BASED_GEOLOCATION=1 ENABLE_DASHBOARD_SUPPORT=0 ENABLE_DATA_TRANSFER_ITEMS=1 ENABLE_DETAILS=1 ENABLE_DEVICE_ORIENTATION=1 ENABLE_DIRECTORY_UPLOAD=1 ENABLE_DOM_STORAGE=1 ENABLE_DOWNLOAD_ATTRIBUTE=1 ENABLE_FILE_SYSTEM=1 ENABLE_FILTERS=1 ENABLE_FULLSCREEN_API=1 ENABLE_GAMEPAD=1 ENABLE_GEOLOCATION=1 ENABLE_GESTURE_EVENTS=1 ENABLE_GESTURE_RECOGNIZER=1 ENABLE_ICONDATABASE=0 ENABLE_INDEXED_DATABASE=1 ENABLE_INPUT_SPEECH=1 ENABLE_INPUT_TYPE_COLOR=0 ENABLE_INPUT_TYPE_DATE=0 ENABLE_INPUT_TYPE_DATETIME=0 ENABLE_INPUT_TYPE_DATETIMELOCAL=0 ENABLE_INPUT_TYPE_MONTH=0 ENABLE_INPUT_TYPE_TIME=0 ENABLE_INPUT_TYPE_WEEK=0 ENABLE_JAVASCRIPT_DEBUGGER=1 ENABLE_JAVASCRIPT_I18N_API=1 ENABLE_LINK_PREFETCH=1 ENABLE_MEDIA_STATISTICS=1 ENABLE_MEDIA_STREAM=1 ENABLE_METER_TAG=1 ENABLE_MHTML=1 ENABLE_MUTATION_OBSERVERS=0 ENABLE_NOTIFICATIONS=1 ENABLE_ORIENTATION_EVENTS=0 ENABLE_PAGE_VISIBILITY_API=1 ENABLE_PROGRESS_TAG=1 ENABLE_QUOTA=1 ENABLE_REQUEST_ANIMATION_FRAME=1 ENABLE_RUBY=1 ENABLE_SANDBOX=1 ENABLE_SHARED_WORKERS=1 ENABLE_SKIA_GPU=0 ENABLE_SMOOTH_SCROLLING=1 ENABLE_SQL_DATABASE=1 ENABLE_SVG=1 ENABLE_SVG_FONTS=1 ENABLE_TOUCH_EVENTS=1 ENABLE_TOUCH_ICON_LOADING=0 ENABLE_V8_SCRIPT_DEBUG_SERVER=1 ENABLE_VIDEO=1 ENABLE_VIDEO_TRACK=1 ENABLE_WEBGL=1 ENABLE_WEB_SOCKETS=1 ENABLE_WEB_TIMING=1 ENABLE_WORKERS=1 ENABLE_XHR_RESPONSE_BLOB=1 ENABLE_XPATH=1 ENABLE_XSLT=1 WTF_USE_LEVELDB=1 WTF_USE_BUILTIN_UTF8_CODEC=1 WTF_USE_OPENTYPE_SANITIZER=1 WTF_USE_WEBP=1 WTF_USE_WEBKIT_IMAGE_DECODERS=1 ENABLE_WEB_AUDIO=1 WTF_USE_ACCELERATED_COMPOSITING=1 ENABLE_3D_RENDERING=1 ENABLE_ACCELERATED_2D_CANVAS=1 WTF_USE_WEBAUDIO_FFMPEG=1 ENABLE_REGISTER_PROTOCOL_HANDLER=1 LANGUAGE_JAVASCRIPT V8_BINDING ..\css\RGBColor.idl
Comment 12 Nico Weber 2011-10-11 16:35:35 PDT 
When running python on windows (not under cygwin), os.execv doesn't preserve spaces:

test.py is this: """
#!/usr/bin/python
import sys
print sys.argv
"""

Called through execv:

>>> os.execv('python', ['python', 'test.py', 'a', 'b c'])                       
                                                                                
['test.py', 'a', 'b', 'c']

subprocess mentions that it correctly does quoting for this: http://docs.python.org/library/subprocess.html#converting-argument-sequence

execv() doesn't do this. Let's just use subprocess on win32 as a workaround.
Comment 13 Nico Weber 2011-10-11 16:46:46 PDT 
Created attachment 110605 [details]
Patch
Comment 14 Nico Weber 2011-10-11 16:47:05 PDT 
new patch
Comment 15 Eric Seidel (no email) 2011-10-11 16:49:38 PDT 
Comment on attachment 110605 [details]
Patch

rs=me.
Comment 16 Dirk Pranke 2011-10-11 16:52:00 PDT 
Comment on attachment 110605 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=110605&action=review

> Source/WebCore/WebCore.gyp/scripts/rule_binding.py:132
> +    else:

since os.execvp() doesn't return, you can delete this else and outdent the following code. You can also delete the trailing else, which is just incorrect.
Comment 17 Nico Weber 2011-10-11 17:06:51 PDT 
Created attachment 110611 [details]
Patch
Comment 18 WebKit Review Bot 2011-10-11 17:40:14 PDT 
Comment on attachment 110611 [details]
Patch

Clearing flags on attachment: 110611

Committed r97204: <http://trac.webkit.org/changeset/97204>
Comment 19 WebKit Review Bot 2011-10-11 17:40:21 PDT 
All reviewed patches have been landed.  Closing bug.