The Inbox: Monticello-ul.678.mcz

> On 19.03.2018, at 01:13, [hidden email] wrote:
>
> Levente Uzonyi uploaded a new version of Monticello to project The Inbox:
> http://source.squeak.org/inbox/Monticello-ul.678.mcz
>
> ==================== Summary ====================
>
> Name: Monticello-ul.678
> Author: ul
> Time: 19 March 2018, 1:03:18.137853 am
> UUID: 4d47635e-aa48-43cc-99bb-b8d2f1a5a51f
> Ancestors: Monticello-cmm.677
>
> Store and reuse a shared WebClient instance for each MCHttpRepository. This makes it possible to speed up downloads by reusing TCP connections.
> Disabled by default. Enable it by evaluating [MCHttpRepository useSharedWebClientInstance: true]
>
> =============== Diff against Monticello-cmm.677 ===============
>
> Item was changed:
>  MCFileBasedRepository subclass: #MCHttpRepository
> + instanceVariableNames: 'location user password readerCache indexed webClient'
> + classVariableNames: 'UseSharedWebClientInstance'
> - instanceVariableNames: 'location user password readerCache indexed'
> - classVariableNames: ''
>   poolDictionaries: ''
>   category: 'Monticello-Repositories'!
>
> Item was added:
> + ----- Method: MCHttpRepository class>>useSharedWebClientInstance (in category 'preferences') -----
> + useSharedWebClientInstance
> +
> + <preference: 'Use shared WebClient instance'
> + category: 'Monticello'
> + description: 'When true, use a shared WebClient instance to speed up downloads from MCHttpRepositories. Requires WebClient to be present.'
> + type: #Boolean>
> + ^UseSharedWebClientInstance ifNil: [ false ]!
>
> Item was added:
> + ----- Method: MCHttpRepository class>>useSharedWebClientInstance: (in category 'preferences') -----
> + useSharedWebClientInstance: aBoolean
> +
> + UseSharedWebClientInstance := aBoolean!
>
> Item was changed:
>  ----- Method: MCHttpRepository>>allFileNames (in category 'overriding') -----
>  allFileNames
> +
>   | index |
> + index := self displayProgress: 'Updating ', self description during: [
> + self httpGet: self locationWithTrailingSlash, '?C=M;O=D' arguments: nil ].
> + ^index ifNotNil: [ self parseFileNamesFromStream: index ]!
> - self displayProgress: 'Updating ', self description during:[
> - index := HTTPSocket httpGet: self locationWithTrailingSlash, '?C=M;O=D' args: nil user: self user passwd: self password.
> - ].
> - index isString ifTrue: [NetworkError signal: 'Could not access ', location].
> - ^ self parseFileNamesFromStream: index !
>
> Item was added:
> + ----- Method: MCHttpRepository>>httpGet:arguments: (in category 'private') -----
> + httpGet: url arguments: arguments
> +
> + | progress urlString client  response result |
> + self class useSharedWebClientInstance ifFalse: [
> + ^HTTPSocket httpGet: url args: arguments user: self user passwd: self password ].
> + progress := [ :total :amount |
> + HTTPProgress new
> + total: total;
> + amount: amount;
> + signal: 'Downloading...' ].
> + urlString := arguments
> + ifNil: [ url ]
> + ifNotNil: [
> + | queryString |
> + queryString := (Smalltalk classNamed: #WebUtils) encodeUrlEncodedForm: arguments.
> + (url includes: $?)
> + ifTrue: [ url, '&', queryString ]
> + ifFalse: [ url, '?', queryString ] ].
> + "Acquire webClient by atomically storing it in the client variable and setting its value to nil."
> + client := webClient.
> + webClient := nil.
> + client ifNil: [ client := (Smalltalk classNamed: #WebClient) new ].
> + response := client
> + username: self user;
> + password: self password;
> + httpGet: urlString do: [ :request |
> + request
> + headerAt: 'Authorization' put: 'Basic ', (self user, ':', self password) base64Encoded;
> + headerAt: 'Connection' put: 'Keep-Alive';
> + headerAt: 'Accept' put: '*/*' ].
> + result := (response code between: 200 and: 299)
> + ifFalse: [
> + response content. "Make sure content is read."
> + nil ]
> + ifTrue: [ (RWBinaryOrTextStream with: (response contentWithProgress: progress)) reset ].
> + "Save the WebClient instance for reuse, but only if there is no client cached."
> + webClient  
> + ifNil: [ webClient := client ]
> + ifNotNil: [ client close ].
> + ^result ifNil: [
> + NetworkError signal: 'Could not access ', location.
> + nil ]!
>
> Item was changed:
>  ----- Method: MCHttpRepository>>readStreamForFileNamed:do: (in category 'private') -----
>  readStreamForFileNamed: aString do: aBlock
> +
>   | contents |
> + contents := self displayProgress: 'Downloading ', aString during: [
> + self httpGet: (self urlForFileNamed: aString) arguments: nil ].
> + ^contents ifNotNil: [ aBlock value: contents ]!
> - self displayProgress: 'Downloading ', aString during:[
> - contents := HTTPSocket httpGet: (self urlForFileNamed: aString) args: nil user: self user passwd: self password.
> - ].
> - ^ contents isString ifFalse: [aBlock value: contents]!
>
>

> Levente Uzonyi uploaded a new version of Monticello to project The Inbox:
> http://source.squeak.org/inbox/Monticello-ul.678.mcz
>
> ==================== Summary ====================
>
> Name: Monticello-ul.678
> Author: ul
> Time: 19 March 2018, 1:03:18.137853 am
> UUID: 4d47635e-aa48-43cc-99bb-b8d2f1a5a51f
> Ancestors: Monticello-cmm.677
>
> Store and reuse a shared WebClient instance for each MCHttpRepository. This makes it possible to speed up downloads by reusing TCP connections.
> Disabled by default. Enable it by evaluating [MCHttpRepository useSharedWebClientInstance: true]
>
> =============== Diff against Monticello-cmm.677 ===============
>
> Item was changed:
>   MCFileBasedRepository subclass: #MCHttpRepository
> +       instanceVariableNames: 'location user password readerCache indexed webClient'
> +       classVariableNames: 'UseSharedWebClientInstance'
> -       instanceVariableNames: 'location user password readerCache indexed'
> -       classVariableNames: ''
>         poolDictionaries: ''
>         category: 'Monticello-Repositories'!
>
> Item was added:
> + ----- Method: MCHttpRepository class>>useSharedWebClientInstance (in category 'preferences') -----
> + useSharedWebClientInstance
> +
> +       <preference: 'Use shared WebClient instance'
> +               category: 'Monticello'
> +               description: 'When true, use a shared WebClient instance to speed up downloads from MCHttpRepositories. Requires WebClient to be present.'
> +               type: #Boolean>
> +       ^UseSharedWebClientInstance ifNil: [ false ]!
>
> Item was added:
> + ----- Method: MCHttpRepository class>>useSharedWebClientInstance: (in category 'preferences') -----
> + useSharedWebClientInstance: aBoolean
> +
> +       UseSharedWebClientInstance := aBoolean!
>
> Item was changed:
>   ----- Method: MCHttpRepository>>allFileNames (in category 'overriding') -----
>   allFileNames
> +
>         | index |
> +       index := self displayProgress: 'Updating ', self description during: [
> +               self httpGet: self locationWithTrailingSlash, '?C=M;O=D' arguments: nil ].
> +       ^index ifNotNil: [ self parseFileNamesFromStream: index ]!
> -       self displayProgress: 'Updating ', self description during:[
> -               index := HTTPSocket httpGet: self locationWithTrailingSlash, '?C=M;O=D' args: nil user: self user passwd: self password.
> -       ].
> -       index isString ifTrue: [NetworkError signal: 'Could not access ', location].
> -       ^ self parseFileNamesFromStream: index  !
>
> Item was added:
> + ----- Method: MCHttpRepository>>httpGet:arguments: (in category 'private') -----
> + httpGet: url arguments: arguments
> +
> +       | progress urlString client  response result |
> +       self class useSharedWebClientInstance ifFalse: [
> +               ^HTTPSocket httpGet: url args: arguments user: self user passwd: self password ].
> +       progress := [ :total :amount |
> +               HTTPProgress new
> +                       total: total;
> +                       amount: amount;
> +                       signal: 'Downloading...' ].
> +       urlString := arguments
> +               ifNil: [ url ]
> +               ifNotNil: [
> +                       | queryString |
> +                       queryString := (Smalltalk classNamed: #WebUtils) encodeUrlEncodedForm: arguments.
> +                       (url includes: $?)
> +                               ifTrue: [ url, '&', queryString ]
> +                               ifFalse: [ url, '?', queryString ] ].
> +       "Acquire webClient by atomically storing it in the client variable and setting its value to nil."
> +       client := webClient.
> +       webClient := nil.
> +       client ifNil: [ client := (Smalltalk classNamed: #WebClient) new ].
> +       response := client
> +               username: self user;
> +               password: self password;
> +               httpGet: urlString do: [ :request |
> +                       request
> +                               headerAt: 'Authorization' put: 'Basic ', (self user, ':', self password) base64Encoded;
> +                               headerAt: 'Connection' put: 'Keep-Alive';
> +                               headerAt: 'Accept' put: '*/*' ].
> +       result := (response code between: 200 and: 299)
> +               ifFalse: [
> +                       response content. "Make sure content is read."
> +                       nil ]
> +               ifTrue: [ (RWBinaryOrTextStream with: (response contentWithProgress: progress)) reset ].
> +       "Save the WebClient instance for reuse, but only if there is no client cached."
> +       webClient
> +               ifNil: [ webClient := client ]
> +               ifNotNil: [ client close ].
> +       ^result ifNil: [
> +               NetworkError signal: 'Could not access ', location.
> +               nil ]!
>
> Item was changed:
>   ----- Method: MCHttpRepository>>readStreamForFileNamed:do: (in category 'private') -----
>   readStreamForFileNamed: aString do: aBlock
> +
>         | contents |
> +       contents := self displayProgress: 'Downloading ', aString during: [
> +               self httpGet: (self urlForFileNamed: aString) arguments: nil ].
> +       ^contents ifNotNil: [ aBlock value: contents ]!
> -       self displayProgress: 'Downloading ', aString during:[
> -               contents := HTTPSocket httpGet: (self urlForFileNamed: aString) args: nil user: self user passwd: self password.
> -       ].
> -       ^ contents isString ifFalse: [aBlock value: contents]!
>
>

> Hey Levente,
>
> Persistent TCP connections by default was changed from HTTP 1.0 to
> 1.1.  I know this is just for talking to our own server, but do you
> suppose we are following W3C recommendations for proper HTTP
> negotiation in each scenario, or should we need to declare any
> explicit protocol versions or header options for that?
>
>    https://www.w3.org/Protocols/rfc2616/rfc2616-sec8.html#sec8.1.2.1
>
>
> On Sun, Mar 18, 2018 at 7:13 PM,  <[hidden email]> wrote:
>> Levente Uzonyi uploaded a new version of Monticello to project The Inbox:
>> http://source.squeak.org/inbox/Monticello-ul.678.mcz
>>
>> ==================== Summary ====================
>>
>> Name: Monticello-ul.678
>> Author: ul
>> Time: 19 March 2018, 1:03:18.137853 am
>> UUID: 4d47635e-aa48-43cc-99bb-b8d2f1a5a51f
>> Ancestors: Monticello-cmm.677
>>
>> Store and reuse a shared WebClient instance for each MCHttpRepository. This makes it possible to speed up downloads by reusing TCP connections.
>> Disabled by default. Enable it by evaluating [MCHttpRepository useSharedWebClientInstance: true]
>>
>> =============== Diff against Monticello-cmm.677 ===============
>>
>> Item was changed:
>>   MCFileBasedRepository subclass: #MCHttpRepository
>> +       instanceVariableNames: 'location user password readerCache indexed webClient'
>> +       classVariableNames: 'UseSharedWebClientInstance'
>> -       instanceVariableNames: 'location user password readerCache indexed'
>> -       classVariableNames: ''
>>         poolDictionaries: ''
>>         category: 'Monticello-Repositories'!
>>
>> Item was added:
>> + ----- Method: MCHttpRepository class>>useSharedWebClientInstance (in category 'preferences') -----
>> + useSharedWebClientInstance
>> +
>> +       <preference: 'Use shared WebClient instance'
>> +               category: 'Monticello'
>> +               description: 'When true, use a shared WebClient instance to speed up downloads from MCHttpRepositories. Requires WebClient to be present.'
>> +               type: #Boolean>
>> +       ^UseSharedWebClientInstance ifNil: [ false ]!
>>
>> Item was added:
>> + ----- Method: MCHttpRepository class>>useSharedWebClientInstance: (in category 'preferences') -----
>> + useSharedWebClientInstance: aBoolean
>> +
>> +       UseSharedWebClientInstance := aBoolean!
>>
>> Item was changed:
>>   ----- Method: MCHttpRepository>>allFileNames (in category 'overriding') -----
>>   allFileNames
>> +
>>         | index |
>> +       index := self displayProgress: 'Updating ', self description during: [
>> +               self httpGet: self locationWithTrailingSlash, '?C=M;O=D' arguments: nil ].
>> +       ^index ifNotNil: [ self parseFileNamesFromStream: index ]!
>> -       self displayProgress: 'Updating ', self description during:[
>> -               index := HTTPSocket httpGet: self locationWithTrailingSlash, '?C=M;O=D' args: nil user: self user passwd: self password.
>> -       ].
>> -       index isString ifTrue: [NetworkError signal: 'Could not access ', location].
>> -       ^ self parseFileNamesFromStream: index  !
>>
>> Item was added:
>> + ----- Method: MCHttpRepository>>httpGet:arguments: (in category 'private') -----
>> + httpGet: url arguments: arguments
>> +
>> +       | progress urlString client  response result |
>> +       self class useSharedWebClientInstance ifFalse: [
>> +               ^HTTPSocket httpGet: url args: arguments user: self user passwd: self password ].
>> +       progress := [ :total :amount |
>> +               HTTPProgress new
>> +                       total: total;
>> +                       amount: amount;
>> +                       signal: 'Downloading...' ].
>> +       urlString := arguments
>> +               ifNil: [ url ]
>> +               ifNotNil: [
>> +                       | queryString |
>> +                       queryString := (Smalltalk classNamed: #WebUtils) encodeUrlEncodedForm: arguments.
>> +                       (url includes: $?)
>> +                               ifTrue: [ url, '&', queryString ]
>> +                               ifFalse: [ url, '?', queryString ] ].
>> +       "Acquire webClient by atomically storing it in the client variable and setting its value to nil."
>> +       client := webClient.
>> +       webClient := nil.
>> +       client ifNil: [ client := (Smalltalk classNamed: #WebClient) new ].
>> +       response := client
>> +               username: self user;
>> +               password: self password;
>> +               httpGet: urlString do: [ :request |
>> +                       request
>> +                               headerAt: 'Authorization' put: 'Basic ', (self user, ':', self password) base64Encoded;
>> +                               headerAt: 'Connection' put: 'Keep-Alive';
>> +                               headerAt: 'Accept' put: '*/*' ].
>> +       result := (response code between: 200 and: 299)
>> +               ifFalse: [
>> +                       response content. "Make sure content is read."
>> +                       nil ]
>> +               ifTrue: [ (RWBinaryOrTextStream with: (response contentWithProgress: progress)) reset ].
>> +       "Save the WebClient instance for reuse, but only if there is no client cached."
>> +       webClient
>> +               ifNil: [ webClient := client ]
>> +               ifNotNil: [ client close ].
>> +       ^result ifNil: [
>> +               NetworkError signal: 'Could not access ', location.
>> +               nil ]!
>>
>> Item was changed:
>>   ----- Method: MCHttpRepository>>readStreamForFileNamed:do: (in category 'private') -----
>>   readStreamForFileNamed: aString do: aBlock
>> +
>>         | contents |
>> +       contents := self displayProgress: 'Downloading ', aString during: [
>> +               self httpGet: (self urlForFileNamed: aString) arguments: nil ].
>> +       ^contents ifNotNil: [ aBlock value: contents ]!
>> -       self displayProgress: 'Downloading ', aString during:[
>> -               contents := HTTPSocket httpGet: (self urlForFileNamed: aString) args: nil user: self user passwd: self password.
>> -       ].
>> -       ^ contents isString ifFalse: [aBlock value: contents]!
>>
>>

>
>> If you mean that these changes are unnecessary, because HTTP/1.1 already
>> defaults to persistent connections, then no, they are not. With the current
>> implementation, the client will close the socket once the request has
>> completed, and it will open a new connection for each subsequent request.
>
> Right.  I didn't mean to imply the change was unnecessary, I'm just
> learning because I know you're an expert in this area.
>
>> If you think the speedup is negligible, then I suggest you backport it to a
>> 5.1 (or older) image (really easy, just file out the changes between
>> Monticello-cmm.677 and Monticello-ul.678 and file them into the 5.1 image)
>> and measure the difference in updating the image to a certain point (to
>> avoid broken updates and merges).
>
> Early versions of Magma opened a socket for each request, it was
> unbearable.  It doesn't use HTTP, but fixing it to reuse the socket
> made a huge improvement in resource utilization and speed, as I know
> this change will for updating from trunk, too.
>
> I need to learn HTTP best practices.
>

> On 21.03.2018, at 02:43, Levente Uzonyi <[hidden email]> wrote:
>
> On Tue, 20 Mar 2018, Chris Muller wrote:
>
>>> The code includes Connection: Keep-Alive header to support servers that
>>> implement HTTP/1.0 only.
>>
>> Great.  That just leaves the question whether the server is running
>> 1.0 or 1.1.  I should know that..
>
> As far as I know, the proxy server we use supports both and 2.0 too.

>
> Levente
>
>>
>>> If you mean that these changes are unnecessary, because HTTP/1.1 already
>>> defaults to persistent connections, then no, they are not. With the current
>>> implementation, the client will close the socket once the request has
>>> completed, and it will open a new connection for each subsequent request.
>>
>> Right.  I didn't mean to imply the change was unnecessary, I'm just
>> learning because I know you're an expert in this area.
>>
>>> If you think the speedup is negligible, then I suggest you backport it to a
>>> 5.1 (or older) image (really easy, just file out the changes between
>>> Monticello-cmm.677 and Monticello-ul.678 and file them into the 5.1 image)
>>> and measure the difference in updating the image to a certain point (to
>>> avoid broken updates and merges).
>>
>> Early versions of Magma opened a socket for each request, it was
>> unbearable.  It doesn't use HTTP, but fixing it to reuse the socket
>> made a huge improvement in resource utilization and speed, as I know
>> this change will for updating from trunk, too.
>>
>> I need to learn HTTP best practices.
>>
>

>
>> On 21.03.2018, at 02:43, Levente Uzonyi <[hidden email]> wrote:
>>
>> On Tue, 20 Mar 2018, Chris Muller wrote:
>>
>>>> The code includes Connection: Keep-Alive header to support servers that
>>>> implement HTTP/1.0 only.
>>>
>>> Great.  That just leaves the question whether the server is running
>>> 1.0 or 1.1.  I should know that..
>>
>> As far as I know, the proxy server we use supports both and 2.0 too.
>
> Yes.
>
>> The proxy will communicate with the image with whatever version the image supports (probably 1.0), but that's less important regarding throughput.
>
> Also, If necessary I can make some changes  so that the RevProx can serve the files directly...
> but not now ;)

>
> -t
>
>>
>> Levente
>>
>>>
>>>> If you mean that these changes are unnecessary, because HTTP/1.1 already
>>>> defaults to persistent connections, then no, they are not. With the current
>>>> implementation, the client will close the socket once the request has
>>>> completed, and it will open a new connection for each subsequent request.
>>>
>>> Right.  I didn't mean to imply the change was unnecessary, I'm just
>>> learning because I know you're an expert in this area.
>>>
>>>> If you think the speedup is negligible, then I suggest you backport it to a
>>>> 5.1 (or older) image (really easy, just file out the changes between
>>>> Monticello-cmm.677 and Monticello-ul.678 and file them into the 5.1 image)
>>>> and measure the difference in updating the image to a certain point (to
>>>> avoid broken updates and merges).
>>>
>>> Early versions of Magma opened a socket for each request, it was
>>> unbearable.  It doesn't use HTTP, but fixing it to reuse the socket
>>> made a huge improvement in resource utilization and speed, as I know
>>> this change will for updating from trunk, too.
>>>
>>> I need to learn HTTP best practices.
>>>
>>