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

WIP DO NOT MERGE: Replacing GetTickCount with GetTickCount64 #667

Closed

Conversation

adrianbielsa1
Copy link
Collaborator

La idea es cambiar la función GetTickCount por GetTickCount64, que sólo está disponible en sistemas de 64 bits pero tiene límites de tiempo más altos (los de la versión de 32 bits rondan los 49 días, pero considerando que VB6 no tiene variables de tipo unsigned, puede que estemos usando sólo la mitad de ese límite).

Esto puede ser una posible solución a #664, pero dada la poca información que tenemos al respecto, lo mejor sería probar la solución unos días y verificar si el error vuelve a suceder. Si pasa, vamos a necesitar poner más logs y revisiones para ver dónde se está generando el error y por qué.

IMPORTANTE: hay que verificar que el código compile y que el servidor arranque correctamente antes de deployar este cambio.

Copy link

@codescene-delta-analysis codescene-delta-analysis bot left a comment

Choose a reason for hiding this comment

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

Code Health Quality Gates: OK

No scoreable code touched

View detailed results in CodeScene

@morgolock morgolock self-assigned this Dec 6, 2024
@morgolock morgolock force-pushed the replace_gettickcount_with_64bits_version branch from d54ea68 to 809f3a1 Compare December 10, 2024 22:03
Copy link

@codescene-delta-analysis codescene-delta-analysis bot left a comment

Choose a reason for hiding this comment

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

Code Health Quality Gates: OK

No scoreable code touched

View detailed results in CodeScene

@RecoX
Copy link
Member

RecoX commented Dec 10, 2024

El PR tiene conflictos. @adrianbielsa1 @morgolock

Replaced `GetTickCount` with its 64-bit counterpart (`GetTickCount64`)

WIP: Changing most timestamps from Long to Double
@morgolock morgolock force-pushed the replace_gettickcount_with_64bits_version branch from 809f3a1 to 9c1cde4 Compare December 10, 2024 22:12
Copy link

@codescene-delta-analysis codescene-delta-analysis bot left a comment

Choose a reason for hiding this comment

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

Code Health Quality Gates: OK

No scoreable code touched

View detailed results in CodeScene

@morgolock morgolock changed the title Replaced GetTickCount with its 64-bit counterpart (GetTickCount64) WIP DO NOT MERGE: Replacing GetTickCount with GetTickCount64 Dec 10, 2024
@adrianbielsa1
Copy link
Collaborator Author

@morgolock buen detalle el de los tipos de las variables que guardan el resultado. Lo que sí, ¿no te parece mejor si directamente encapsulamos eso también? Por ej., en vez de que la función GetTickCount devuelva Long o Currency, hacer que devuelva un tipo opaco, algo así:

Public Type tTimestamp
    Value As Currency
End Type

La idea entonces sería que los consumidores no interactúen directamente con el Value de adentro, sino darles funciones para hacer los cálculos que necesiten, por ejemplo:

Public Function millisecondsElapsedSince(ByValue timestamp as tTimestamp) As Long
    millisecondsElapsedSince = GetTickCount().Value - timestamp.Value
End Function

Obviamente esa función estaría dentro del módulo modTime. Si queremos evitar al 100% que usen el Value interno podemos crear una clase, pero no me gusta mucho como idea porque tendrías que hacer una allocation en el heap cada vez que llamas a GetTickCount.

Otra ventaja de hacerlo de esta manera es que todos los lugares donde se esté llamando a GetTickCount se van a volver errores de compilación hasta que hagamos el cambio, por lo que no nos vamos a encontrar con bugs sorpresa por la coerción implícita de tipos en VB6.

Si te parece que es un overkill, entonces diría que en vez de usar Double para las timestamps usemos Currency, que es el tipo de dato que devuelve la función actualmente.

@morgolock
Copy link
Contributor

morgolock commented Dec 12, 2024

@adrianbielsa1 si me parece bien lo que propones, no me gusta la idea de usar una clase por las razones que mencionas. Pero si me gusta lo de usar:

Public Type tTimestamp
    Value As Currency
End Type

El problema ahora es que hay muchas partes en el codigo que usan Long directamente. Si modificamos a usar un nuevo typo tTimestamp eso va a ayudarnos a hacer todos los cambios porque donde se usa Long vamos a tener que reemplazarlo por el nuevo tipo.

@RecoX RecoX deleted the branch master2 January 18, 2025 00:49
@RecoX RecoX closed this Jan 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants